mirror of
https://github.com/ultimatepp/ultimatepp.git
synced 2026-05-15 14:16:07 -06:00
[GH-ISSUE #31] Eliminating warnings and making life easier for future generations #18
Labels
No labels
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: github-starred/ultimatepp#18
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @mingodad on GitHub (Jan 7, 2021).
Original GitHub issue: https://github.com/ultimatepp/ultimatepp/issues/31
I noticed that the IDE hide the compilation commands and it's warnings from the user (I'm used to Codeblocks and Netbeans) so I exported a Makefile for the IDE and created a Netbeans project to build it.
To the exported Makefile I added the following compiler options
-Wall -pipeand noticed lots of warnings related to parentheses around boolean expressions and found this interesting one in uppsrc/ide/RepoSync.cpp:Also to suppress the warnings from GTK-3 headers I added this to uppsrc/CtrlCore/Gtk.h:
I'm trying to fix as much warnings I can and I'll add then here later.
@mingodad commented on GitHub (Jan 7, 2021):
Another joy found in uppsrc/ide/Builders/GccBuilder.cpp:
@mingodad commented on GitHub (Jan 7, 2021):
Another joy in uppsrc/Core/Socket.cpp:
@mingodad commented on GitHub (Jan 7, 2021):
Today in Lua mailing list there is several messages about operator precedence here is one interesting message:
@mingodad commented on GitHub (Jan 7, 2021):
There is this warning that probably need a review:
@mingodad commented on GitHub (Jan 7, 2021):
Here is the changes I made to eliminate the compiler warnings (that ended up messing up a bit see the screenshot attached where we can see that the editor comes up with an HEX view of the file):
@mingodad commented on GitHub (Jan 7, 2021):
Redoing it with the help of clang11:
@mingodad commented on GitHub (Jan 7, 2021):
This one also need review:
@mingodad commented on GitHub (Jan 7, 2021):
This one also need review:
@mingodad commented on GitHub (Jan 7, 2021):
And here is the second replacement I did again from scratch and now I get the files shown properly in the built IDE:
@klugier commented on GitHub (Jan 7, 2021):
@mingodad The reason why it is still there is that Mirek doesn't' like it, so we always use "-Wno-logical-op-parentheses" in context of clang (because there was a time that it was enable by default, they hopefully changed that in the current releases). Anyway, please do not post patches with that fixes. We could accept other fixes related to -Wall, but not this particular one.
When you exporting makefile I highly suggest to use "Wno-logical-op-parentheses", so you should be free from these warnings.
@mingodad commented on GitHub (Jan 7, 2021):
And here is a diff between the first and the second chages:
@mingodad commented on GitHub (Jan 7, 2021):
As you could see doing so we end up with silly bugs that the compiler could point to us.
With this second attempt there is no warnings with clang and a few ones with g++ (related to signed/uunsigned and few operations with bitwise).
I hope the decision pointed out by Mirek get reconsidered !
@mingodad commented on GitHub (Jan 7, 2021):
Also for readability and maintainability as put in the title
it'll make life easier for future generations(of new developers and ourselves).And my first attempt is a testimony of it, I've got several of my interpretation of the warnings wrong, the second time following the good warning messages of clang11 I'm more confident that the second attempt is a lot more correct (what doesn't endorse that all of then are correct based on the intention of the original writer (that probably got confused in several of then)).
@klugier commented on GitHub (Jan 7, 2021):
I do not thing it happened - please notice that potential fix might cause regression and it is very likely to happen. There are dozens of places that will need to change. How, to test them all? We have automation, but it is not enough. We will need army of Quality Engineers to complete the whole process. So, I do not see any benefits of doing it right now. It will cause more problems, that it will gain.
You can always silence it by ""Wno-logical-op-parentheses". Anyway you can still enable it for your code by passing "-Wall" per package or enabling it globaly and passing "Wlogical-op-parentheses".
I am closing this thread - you could post the diff for others "-Wall" warning, but the parenthesis will stay like it is right now. I will write to Mirek about it, so if he deiced that want's to do something with it he will reopen.
Of course, if you do not agree, you could always create fork free of this issue :)
@mingodad commented on GitHub (Jan 7, 2021):
There is ways to gain new contributors and ways to gain/retain technical debt !
@klugier commented on GitHub (Jan 7, 2021):
I totally agree that fixing all warnings under (-Wall flag or equivalent in other compilers) is the good practice and professional projects should follow them. This is also one of the most important security practice https://wiki.sei.cmu.edu/confluence/display/seccode/Top+10+Secure+Coding+Practices. Also, -Wall is not sufficient to detect all issues in given code. Previously I used tools such as flawfinder for finding security and believe me upp has a lot of warnings. The main problem here is that upp is the leader base project - it means that the key decision are made by single person - Mirek. And, if he has strong opinion about something then it will be very hard to change it on the main/master branch. On the other hand upp is open source, so if you do not agree you can create fork and make any changes you want. However, this will cost you - you will need to spend additional time on back-propagating issues, conflict resolution etc. The effort is not worth it. So, during the years in this project I learned that some things are unchangeable like this warning with parenthesis. I had several discussion with Mirek about this and to this day nothing changed. So, I live with it. I have nice easy to use GUI framework. Much easier and in many places better than these available on the market, which I can fork any time I want...
Anyway, I like that you raised so many issues. Most of them are valuable and should be fixed. I hope we will manage to fix other warnings too. So, if you post the fix to other warnings I would be grateful.
Thanks for the valuable discussion and yes I am the enemy of the tech debt. It ruined not one project.