clean release build output of benign warnings: -Wempty-body, -Wmaybe-unitialized, -Wunused-variable, -Wstrict-overflow
I get these same sets of warnings on my x86-64 and ARMv7 arch linux machines when building RELEASE:
- The two -Wempty-body deal with a Q_ASSERT statement inside of an else that doesn't have brackets. Release build removes that assert, and hence nothing is inside the else.
- For the -Wmaybe-uninitialized I need your help to figure out reasonable initial defaults.
- The -Wunused-variable refers to Ms::updatePeriods in preferences.cpp, which I'm going to comment out of the code, so incase someone later wants to return to using those strings, then it will still be in the code, just commented out.
- The -Wstrict-overflow I need to figure out the best way to tell the compiler that it is ok to not be concerned about the overflow. Either suppress the warning in makefile or change something in the code...
After I remove those warnings, then I should have a clean release build.
Comments
I'm attaching my make release output based on latest git master 5b20a4a on both x86-64 and ARMv7:
We may not be able to do much about that "Q_ASSERT(false);" generating a -Wempty-body warning, this might be a bogus Marco in Qt. Maybe "Q_ASSERT((false));" fixes it though.
The unitialized variables, well, better initialize them (probably best to 0 resp. 0.0), but I wonder why other platforms don't warn about them?
In some cases this is probably due to a switch statement without a proper default case.
That unused variable, I'd just delete of ifdef 0 the entire PeriorItem section
"Q_ASSERT((false));" doesn't fix that.
If I put braces around it as the compiler suggests:
else { Q_ASSERT(false); }Then the warning is removed, but now I'm not obeying the musescore coding rules about braces.
I'll set the uninitialized to 0. You say you wonder why other platforms don't warn about them, but I also see my Raspberry Pi running Debian Jessie complain about them in addition to my Arch machines. What other platforms are you talking about...win/mac/ubuntus?
>>That unused variable, I'd just delete of ifdef 0 the entire PeriorItem section
Ok, I'll enclose with #ifdef 0...#endif
Windows and whatever travis is running, I think Mac too, no warnings there
I'd go for that {QASSERT(false)}, with a suitable comment maybe, either in source code or the commit message or both?
and "#if 0", not "#ifdef 0", please
no idea what to do about the -Wstrict-overflow
I want to ask is it possible (or even desired) to disable -Wstrict-overflow and -Wempty-body in the makefiles? I sortof don't want to do that because might suppress a future error, but I thought I would ask.
Try
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstrict-overflow"
and maybe
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wempty-body"
before and
#pragma GCC diagnostic pop
after the offending line(s) of code
See https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
thanks, that pragma did the trick. Although I've decided to just use that pragma for the -Wstrict-overflow because I can enclose the whole function without disrupting the look of the code (and I believe the optimization covers more than a single line)...here's what that looks like now:
https://github.com/ericfont/MuseScore/commit/c87ce8d206e660cbd61b8544c7…
But for the -Wempty-body, I'm just using the {Q_ASSERT(false);} since I think the pragma would disrupt the look of the code too much:
https://github.com/ericfont/MuseScore/commit/2b8e5a030a1f6f3566225b7a8e…
I agree
ok...I quickly idiot-checked the -Wmaybe-uninitialized and couldn't spot any possible coding errors, so I've initialized them to 0 here:
https://github.com/musescore/MuseScore/commit/ad1cb3d1bf30e6c546c5a7e32…
and I surrounded the PeriodItem updatePeriods[] with #if 0 and #endif like this:
https://github.com/musescore/MuseScore/commit/8a594897c337ba43830c51a6b…
So I think I'm ready to squash and submit PR...let me know. I'm going to double check with "make clean && make release" so I know I caught everything.
I've squashed and submitted PR:
https://github.com/musescore/MuseScore/pull/2376
Thanks Jojo for your assistance.
I'm verifying that compiler no longer produces any warnings both on my x86-64 and ARMv7-A archlinux machines (output attached) after I apply this PR and my PR for [[nodetitle:#96951]] on a clean make release.
Fixed in branch master, commit 7aa8c7e6cd
fix #96971 suppress benign make release warnings
This commit removes some -Wempty-body, -Wmaybe-uninitialized, -Wunused-variable, and -Wstrict-overflow compiler warnings that arise on my x86-64 and ARM arch linux machines when compiling release.
These compiler warnings don't seem to cause any bugs, but since they pollute the build output, they make it harder to spot potentially important warnings that might arise, so I'm making these small changes to keep the build output clean.
The "empty-body" warnings occur because the Q_ASSERT statements are removed for release compiles, causing the else blocks to be empty. Surrounding the Q_ASSERT with brackets will let the compiler know we aren't unintentionally having an empty else body.
The "maybe-uninitialized" warnings are handled by assigning the variables to 0 either at initialization or in a switch default block.
The "unused-variable" warning is due to PeriodItem updatePeriods[] in preferences.cpp being defined but never used, so I've removed PeriodItem. Also a recent commit has an unused "int staves", which I've removed.
The "strict-overflow" warning is due to compiler wanting to perform an optimization which would cause signed overflow during a comparison operation. I've made the compiler happy by casting the barIndex'es into unsigned ints, so it doesn't have to worry.
Fixed in branch master, commit 61b0757091
Merge pull request #2376 from ericfont/96971-clean-benign-release-build-warnings
fix #96971 suppress benign make release warnings
Fixed in branch 2.0.3, commit 10a9a8e113
fix #96971 suppress benign make release warnings
This commit removes some -Wempty-body, -Wmaybe-uninitialized, -Wunused-variable, and -Wstrict-overflow compiler warnings that arise on my x86-64 and ARM arch linux machines when compiling release.
These compiler warnings don't seem to cause any bugs, but since they pollute the build output, they make it harder to spot potentially important warnings that might arise, so I'm making these small changes to keep the build output clean.
The "empty-body" warnings occur because the Q_ASSERT statements are removed for release compiles, causing the else blocks to be empty. Surrounding the Q_ASSERT with brackets will let the compiler know we aren't unintentionally having an empty else body.
The "maybe-uninitialized" warnings are handled by assigning the variables to 0 either at initialization or in a switch default block.
The "unused-variable" warning is due to PeriodItem updatePeriods[] in preferences.cpp being defined but never used, so I've removed PeriodItem. Also a recent commit has an unused "int staves", which I've removed.
The "strict-overflow" warning is due to compiler wanting to perform an optimization which would cause signed overflow during a comparison operation. I've made the compiler happy by casting the barIndex'es into unsigned ints, so it doesn't have to worry.
Automatically closed -- issue fixed for 2 weeks with no activity.