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 608953 - Add some more compiler warning options
Add some more compiler warning options
Status: RESOLVED FIXED
Product: gnome-common
Classification: Core
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: David King
Gnome Common Maintainer(s)
: 568546 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-02-04 03:32 UTC by Javier Jardón (IRC: jjardon)
Modified: 2012-11-06 20:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add more compiler warning flags (1.50 KB, patch)
2010-02-04 03:40 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
add warnings and check whether they are supported (4.61 KB, patch)
2012-07-28 15:13 UTC, David King
none Details | Review
updated patch with original indentation (4.16 KB, patch)
2012-07-29 12:50 UTC, David King
none Details | Review
modules using gnome-common in current gnome-ostree build (520 bytes, text/plain)
2012-08-05 21:03 UTC, Colin Walters
  Details
updated, make aggregate return values warning non-fatal (4.15 KB, patch)
2012-08-07 07:58 UTC, David King
none Details | Review
updated, make declaration after statement only a warning at maximum (4.15 KB, patch)
2012-08-14 06:32 UTC, David King
committed Details | Review

Description Javier Jardón (IRC: jjardon) 2010-02-04 03:32:45 UTC
I think that would be great add some useful compiler warnings flags to the GNOME_COMPILE_WARNINGS() macro when "maximum" option is used


Some resources:

http://blogs.gnome.org/otte/2008/12/22/warning-options/
http://mces.blogspot.com/2008/12/year-end-cleaning-ie-on-warning-options.html
http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Comment 1 Javier Jardón (IRC: jjardon) 2010-02-04 03:40:45 UTC
Created attachment 152987 [details] [review]
Add more compiler warning flags

I've used -Wno-missing-field-initializers and -Wno-unused-parameter because, as pointed here [1]:

- "funcs in classes and signal callbacks often have parameters that you don’t use"
- "It’s usual glib coding style to do just that:
GValue val = { 0, };"

[1] http://blogs.gnome.org/otte/2008/12/22/warning-options/
Comment 2 Benjamin Otte (Company) 2010-02-05 13:18:35 UTC
I'm all for it!
Note however that those compiler flags are very current (like introduced-in-gcc4.3-current) so you should check most/all of them. Also someone should check the list of new additions in gcc 4.4 and add them.
A comment about http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36750 would be nice, too.
Comment 3 Behdad Esfahbod 2010-02-10 19:39:28 UTC
+1.  We have autofoo code in cairo to test whether a warning flag is supported (with caching and all).
Comment 4 David King 2010-04-15 20:04:53 UTC
As an example of how checking for compiler flags might be achieved, mm-common has a macro that does exactly this:

http://git.gnome.org/browse/mm-common/tree/macros/mm-warnings.m4
Comment 5 Murray Cumming 2011-06-15 11:54:30 UTC
More importantly, we need some way for application developer to specify their own set of compiler warning flags, and their own set of deprecation defines. The macros are not very useful to real projects without this.

(mm-common's macros let you specify them.)
Comment 6 Murray Cumming 2011-06-15 11:57:12 UTC
See also bug #568546 about adding -Werror=format-security
Comment 7 David King 2012-07-28 15:13:00 UTC
Created attachment 219785 [details] [review]
add warnings and check whether they are supported

The attached patch adds more warnings and also checks whether they are supported by GCC. It should solve the problems that Colin Walters brought up on desktop-devel-list:

https://mail.gnome.org/archives/desktop-devel-list/2012-July/msg00100.html

This might lead to quite a bit of breakage, so probably needs a bit of discussion and review.
Comment 8 Colin Walters 2012-07-28 21:19:19 UTC
So I think a clear step before we land is to just try a test build with this.  I'll try it.
Comment 9 Luis Menina 2012-07-29 10:23:13 UTC
@Dave (In reply to comment #7)
> Created an attachment (id=219785) [details] [review]
> add warnings and check whether they are supported

Just a quick comment about the patch: seems you messed up indentation.
Comment 10 David King 2012-07-29 12:50:05 UTC
Created attachment 219837 [details] [review]
updated patch with original indentation

Thanks Luis, I fixed the indentation.
Comment 11 David King 2012-08-04 14:20:53 UTC
(In reply to comment #8)
> So I think a clear step before we land is to just try a test build with this. 
> I'll try it.

Colin, do you have the results of a test build? I would be happy to go through and fix the warnings and failures that came up.
Comment 12 Colin Walters 2012-08-05 21:03:04 UTC
Created attachment 220399 [details]
modules using gnome-common in current gnome-ostree build

List of modules in current gnome-ostree manifest that use gnome-common.
Comment 13 Colin Walters 2012-08-05 21:16:03 UTC
First failure I get is gnome-desktop:

| ../../libgnome-desktop/gnome-thumbnail-pixbuf-utils.c: In function 'gnome_desktop_thumbnail_scale_down_pixbuf':
| ../../libgnome-desktop/gnome-thumbnail-pixbuf-utils.c:80:6: error: function call has aggregate value [-Werror=aggregate-return]
| ../../libgnome-desktop/gnome-thumbnail-pixbuf-utils.c:84:6: error: function call has aggregate value [-Werror=aggregate-return]
| cc1: some warnings being treated as errors

I don't think we should make that one into a fatal error.  Two/three-element structures are fairly legitimate as return values.
Comment 14 David King 2012-08-07 07:58:11 UTC
Created attachment 220516 [details] [review]
updated, make aggregate return values warning non-fatal
Comment 15 Colin Walters 2012-08-09 22:23:52 UTC
So, gnome-bluetooth hits:

| ../../sendto/obex-agent.c: In function 'obex_agent_setup':
| ../../sendto/obex-agent.c:381:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
| cc1: some warnings being treated as errors

I fixed an example of this in gnome-desktop, but we should probably be fairly conservative with this patch initially...

Note that some modules like gnome-bluetooth we don't care about building on Windows (i.e. MSVC), so using C99 for those is fine.

How about just

-Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=format=2

Oh and we're actually missing

-Werror=format-security

BTW anyone else can try running jhbuild with this patch too!
Comment 16 David King 2012-08-14 06:32:40 UTC
Created attachment 221098 [details] [review]
updated, make declaration after statement only a warning at maximum

OK, I dropped the error on declarations after statements, and made it a warning at the maximum/error level only. It is probably something that is better as a language compliance flag anyway (--enable-iso-c for example). I added -Werror=format-security, and left the remaining flags intact, as they seem scary enough that they should stop the build.

Colin, can you do another test run with the updated patch? If there are only a few modules that break because of the aggressive warning options, I would be happy to merge the patch and fix those modules up, but if there are more than say 5-10 then I will drop to the warnings options that you suggested in comment 15.
Comment 17 Behdad Esfahbod 2012-08-21 18:20:14 UTC
I highly suggest also including -fno-common.  That would avoid errors like this:

http://git.gnome.org/browse/at-spi2-core/commit/?id=c847823f73046f928ae942a37bd97ae5ca1501ae
Comment 18 Colin Walters 2012-08-22 01:50:37 UTC
Review of attachment 221098 [details] [review]:

This patch builds all the gnome-ostree manifest for me.  We're a bit late in the release cycle though (sorry about being slow on trying this in my builds).  Should we defer this for 3.7 or just go with it now?  I'll ping the release-team.
Comment 19 Javier Jardón (IRC: jjardon) 2012-08-22 02:14:42 UTC
(In reply to comment #18)
> Review of attachment 221098 [details] [review]:
> 
> This patch builds all the gnome-ostree manifest for me.  We're a bit late in
> the release cycle though (sorry about being slow on trying this in my builds). 
> Should we defer this for 3.7 or just go with it now?  I'll ping the
> release-team.

+1 from me (RT member), but please ping maintainers to update their gnome-common when this gets pushed
Comment 20 Colin Walters 2012-10-31 21:50:42 UTC
We're in 3.7 now, most modules are branched.  Let's try again with this?  I can do another round of build testing.

Any objections?
Comment 21 David King 2012-11-01 00:02:22 UTC
Sure Colin, please do another quick round of testing and then we can get this merged if the results still look good.
Comment 22 Colin Walters 2012-11-02 16:12:53 UTC
Review of attachment 221098 [details] [review]:

Everything in the gnome-ostree 3.8 manifest builds.
Comment 23 David King 2012-11-02 16:29:10 UTC
Comment on attachment 221098 [details] [review]
updated, make declaration after statement only a warning at maximum

Fantastic! Pushed to master as commit e42ca9bec9d83aa53d7f79c480daf4b7c4d95fe1.
Comment 24 Allison Karlitskaya (desrt) 2012-11-06 19:10:16 UTC
eog and anjuta (at least) are currently breaking jhbuild due to this.
Comment 25 Allison Karlitskaya (desrt) 2012-11-06 19:11:37 UTC
brasero as well...
Comment 27 David King 2012-11-06 20:06:49 UTC
*** Bug 568546 has been marked as a duplicate of this bug. ***
Comment 28 Carl-Anton Ingmarsson 2012-11-06 20:12:12 UTC
The patch at https://bugzilla.gnome.org/show_bug.cgi?id=687664 fixes the anjuta errors for me.