GUI: Enable more warnings#97
Conversation
SynchronousData is non-trivially copyable due to std::vector, so memcpy can't be used. Therefore copying any uninitialized value is ub here.
No new warnings generated this time (on g++)!
Strings longer than 65536 is a non-standard feature, let's allow it so fonts.h compiles.
Warn the user if a class with virtual functions has a non-virtual destructor. This helps catch hard to track down memory errors.
Warn if you overload (not override) a virtual function.
Warn if indentation implies blocks where blocks do not exist.
warn if if / else branches have duplicated code
...and remove the single useless cast it found
Warn on security issues around functions that format output (i.e., `printf`)
|
I noticed we get bunch of warnings with |
Warn about extraneous semicolons.
Warn when a literal '0' is used as null pointer constant.
Warn if an undefined variable is used to initialize itself.
Warn if string literal is implicitly converted to a bool
Suggest adding the override specifier to member functions that override
...and fix the warnings it produced.
Warn about all deprecated features
Warn about suspicious uses of logical operators
Warn if casting removes const or volatile
Warn if casting increases the required alignment of the data
Warn if a class seems unusable due to everything being private
Not a bad idea. But I'd say let's try to go without it for now, and see how things evolve.
I'll take that as a compliment, haha.
Yea, I looked at them, most of them look easy enough, but it's also something that will not take 5 minutes, and I feel like it provides very little actual benefit im most cases. All looks good to me, still compiles, still no warnings (on g++). I gotta admit tho, I'm impressed with the number-of-commits to lines-changed ratio. I feel like we're quite the opposites on this front, as I'm - as you might already know - more of a "30K lines changed - *Minor Changes*"-type of guy. |
|
I was anticipating having to fix more issues, in that case the individual commits would have helped reviewing. But as they stand they don't really add that much yeah |
2 participants
Enable warnings that seemed useful from here: https://github.com/cpp-best-practices/cppbestpractices/blob/master/02-Use_the_Tools_Available.md#gcc--clang. Fix any compilation warnings.
This is quite many, but my thinking is that it's easier to enable warnings when the code is still clean of them, they can just be disabled in the future if they become annoying. Discarding some of them here is also fine to me if any of them seem overzealous / uninteresting for us.
I chose to suppress
-Wunused-parameter. Do we care about this one? Enabling it would mean that we'd have to remove any unused params, or do this:void a(int /*x*/, int /*y*/).One imo interesting warning that I didn't enable was
-Wnull-dereferencebecause I ran into a peculiar problem with it: when using LTO, it's only effective during linking. However we cant really have it enabled for linking, because ImGui produces warnings regarding it. So this remains disabled for now.Note regarding CI: should we promote warnings to errors using
-Werrorexclusively in the pipeline, so the code would remain warning free? Or would that be too annoying?