After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 688192 - Various compiler warning patches
Various compiler warning patches
Status: RESOLVED FIXED
Product: gnome-common
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Gnome Common Maintainer(s)
Gnome Common Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-11-12 17:37 UTC by Colin Walters
Modified: 2015-01-26 15:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
See attached (1.95 KB, patch)
2012-11-12 17:37 UTC, Colin Walters
committed Details | Review
see attached (1.17 KB, patch)
2012-11-12 17:37 UTC, Colin Walters
committed Details | Review
see attached (1.54 KB, patch)
2012-11-12 17:38 UTC, Colin Walters
reviewed Details | Review
compiler-warnings: code cleanup: Extract common warnings into variables (2.35 KB, patch)
2012-11-12 20:24 UTC, Colin Walters
committed Details | Review
compiler-warnings: Move -Wnested-externs to the always-on warning set (1.06 KB, patch)
2012-11-12 20:24 UTC, Colin Walters
committed Details | Review
compiler-warnings: Drop -Wno-sign-comare (1.07 KB, patch)
2012-11-12 20:24 UTC, Colin Walters
committed Details | Review
compiler-warnings: Drop -Wdeclaration-after-statement (1015 bytes, patch)
2012-11-12 20:24 UTC, Colin Walters
committed Details | Review
compiler-warnings: Make strict-prototypes into an explicit error (1.18 KB, patch)
2012-11-12 20:24 UTC, Colin Walters
accepted-commit_now Details | Review

Description Colin Walters 2012-11-12 17:37:34 UTC
Created attachment 228796 [details] [review]
See attached

See attached.
Comment 1 Colin Walters 2012-11-12 17:37:55 UTC
Created attachment 228797 [details] [review]
see attached
Comment 2 Colin Walters 2012-11-12 17:38:13 UTC
Created attachment 228798 [details] [review]
see attached
Comment 3 David King 2012-11-12 18:42:25 UTC
Review of attachment 228796 [details] [review]:

Sure, makes sense.
Comment 4 David King 2012-11-12 18:43:49 UTC
Review of attachment 228797 [details] [review]:

Seems reasonable, go ahead.
Comment 5 David King 2012-11-12 18:59:23 UTC
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.
Comment 6 Colin Walters 2012-11-12 19:40:52 UTC
(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.
Comment 7 Colin Walters 2012-11-12 20:24:01 UTC
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.
Comment 8 Colin Walters 2012-11-12 20:24:03 UTC
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.
Comment 9 Colin Walters 2012-11-12 20:24:05 UTC
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.
Comment 10 Colin Walters 2012-11-12 20:24:08 UTC
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.
Comment 11 Colin Walters 2012-11-12 20:24:11 UTC
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.
Comment 12 Colin Walters 2012-11-12 20:25:35 UTC
These split out 5 patches achieve much the same end as the obsoleted one, but we can look at them more individually.
Comment 13 David King 2012-11-12 20:32:25 UTC
Review of attachment 228824 [details] [review]:

Looks good, just check tabs versus spaces for indentation.
Comment 14 David King 2012-11-12 20:33:38 UTC
Review of attachment 228825 [details] [review]:

OK, again check indentation.
Comment 15 David King 2012-11-12 20:34:42 UTC
Review of attachment 228826 [details] [review]:

Sure, just fix the typo in the commit message.
Comment 16 David King 2012-11-12 20:35:24 UTC
Review of attachment 228827 [details] [review]:

Seems reasonable, and it is easy for a module to add this themselves, as you say.
Comment 17 David King 2012-11-12 20:36:29 UTC
Review of attachment 228828 [details] [review]:

Seems fine.
Comment 18 Colin Walters 2012-11-13 17:57:24 UTC
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.
Comment 19 Colin Walters 2012-11-13 17:59:33 UTC
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
Comment 20 Philip Withnall 2014-05-02 14:58:36 UTC
(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?
Comment 21 Philip Withnall 2015-01-16 10:42:44 UTC
(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?
Comment 22 David King 2015-01-26 15:45:16 UTC
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.