GNOME Bugzilla – Bug 688192
Various compiler warning patches
Last modified: 2015-01-26 15:45:16 UTC
Created attachment 228796 [details] [review] See attached See attached.
Created attachment 228797 [details] [review] see attached
Created attachment 228798 [details] [review] see attached
Review of attachment 228796 [details] [review]: Sure, makes sense.
Review of attachment 228797 [details] [review]: Seems reasonable, go ahead.
Review of attachment 228798 [details] [review]: Not sure I agree with this change, other than to be kind to all those who do "GNOME_COMPILE_WARNINGS([maximum])" in configure.ac. Is that the motivation? It seems reasonable to me to keep -Wnested-externs and -Wdeclaration-after-statement for the maximum/error case. As for -Wno-sign-compare, I would be tempted to invert that to be -Wsign-compare at the maximum/error level. Maybe a better solution is to go ahead with this change and add a new warning level "maximum-extra" or similar for the more exotic (or controversial) warnings.
(In reply to comment #5) > Review of attachment 228798 [details] [review]: > > Not sure I agree with this change, other than to be kind to all those who do > "GNOME_COMPILE_WARNINGS([maximum])" in configure.ac. Is that the motivation? It was more simply that they were so close as is. > It seems reasonable to me to keep -Wnested-externs I had to look this one up...no objections to keeping it. > and > -Wdeclaration-after-statement for the maximum/error case. This one is more controversial - a lot more. As I mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=608953#c15 there are GNOME modules that want to use C99, and we shouldn't get in their way. I think for the GTK+ stack and below which needs to be MSVC portable (and unfortunately at the moment doesn't use gnome-common anyways), we should have them add this in their own module. > As for > -Wno-sign-compare, I would be tempted to invert that to be -Wsign-compare at > the maximum/error level. This one can involve some seriously nontrivial work to fix if your code is heavily based around parsing byte arrays and uses "char *" (note: signedness varies per platform). I tried to fix it in DBus at one time, and just gave up. This kind of thing is the reason it's not in -Wall I'd guess. > Maybe a better solution is to go ahead with this > change and add a new warning level "maximum-extra" or similar for the more > exotic (or controversial) warnings. At the high level, there's two ways to go: * gnome-common is the "mininum GNOME quality base". So we focus on high-value warnings like -Werror=implicit-function-declaration which are just not in -Wall for basically historical reasons, and leave up to individual modules to add stuff like -Werror=sign-compare on their own. * gnome-common contains several levels of options, from the minimum base above to levels which have -Werror=sign-compare, etc. Regardless though, let me do individual patches for items above on which we agree.
Created attachment 228824 [details] [review] compiler-warnings: code cleanup: Extract common warnings into variables Will make future refactoring clearer, and also we have comments now.
Created attachment 228825 [details] [review] compiler-warnings: Move -Wnested-externs to the always-on warning set Since there's no reason to have it different for yes/maximum.
Created attachment 228826 [details] [review] compiler-warnings: Drop -Wno-sign-comare It's not part of -Wall, and we're not explicitly turning it on here, so there's no point in turning it off, since it's not on. Additionally, if a given module did want it on, it's clearer if the compiler flags don't have -Wno-sign-compare -Wsign-compare.
Created attachment 228827 [details] [review] compiler-warnings: Drop -Wdeclaration-after-statement Some GNOME modules want the ability to use C99, let's not hamper them.
Created attachment 228828 [details] [review] compiler-warnings: Make strict-prototypes into an explicit error Since we have missing-prototypes here, and such, strict prototypes goes alongside that.
These split out 5 patches achieve much the same end as the obsoleted one, but we can look at them more individually.
Review of attachment 228824 [details] [review]: Looks good, just check tabs versus spaces for indentation.
Review of attachment 228825 [details] [review]: OK, again check indentation.
Review of attachment 228826 [details] [review]: Sure, just fix the typo in the commit message.
Review of attachment 228827 [details] [review]: Seems reasonable, and it is easy for a module to add this themselves, as you say.
Review of attachment 228828 [details] [review]: Seems fine.
Ok, I actually haven't tested a gnome-ostree rebuild with -Werror=strict-prototypes - I'll push everything except that one, do the test, and push the last one after any code has been fixed for it.
Attachment 228824 [details] pushed as afad58c - compiler-warnings: code cleanup: Extract common warnings into variables Attachment 228825 [details] pushed as b90f3e5 - compiler-warnings: Move -Wnested-externs to the always-on warning set Attachment 228826 [details] pushed as eeca390 - compiler-warnings: Drop -Wno-sign-comare Attachment 228827 [details] pushed as 1e23c48 - compiler-warnings: Drop -Wdeclaration-after-statement
(In reply to comment #18) > Ok, I actually haven't tested a gnome-ostree rebuild with > -Werror=strict-prototypes - I'll push everything except that one, do the test, > and push the last one after any code has been fixed for it. Colin, have you tested this yet?
(In reply to comment #20) > (In reply to comment #18) > > Ok, I actually haven't tested a gnome-ostree rebuild with > > -Werror=strict-prototypes - I'll push everything except that one, do the test, > > and push the last one after any code has been fixed for it. > > Colin, have you tested this yet? Ping?
Let's mark this as fixed. We are not quite sure whether strict-prototypes is suitable to be an error, so we may or may not add that to the autoconf-archive equaivalent.