GNOME Bugzilla – Bug 608953
Add some more compiler warning options
Last modified: 2012-11-06 20:12:12 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
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/
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.
+1. We have autofoo code in cairo to test whether a warning flag is supported (with caching and all).
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
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.)
See also bug #568546 about adding -Werror=format-security
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.
So I think a clear step before we land is to just try a test build with this. I'll try it.
@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.
Created attachment 219837 [details] [review] updated patch with original indentation Thanks Luis, I fixed the indentation.
(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.
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.
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.
Created attachment 220516 [details] [review] updated, make aggregate return values warning non-fatal
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!
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.
I highly suggest also including -fno-common. That would avoid errors like this: http://git.gnome.org/browse/at-spi2-core/commit/?id=c847823f73046f928ae942a37bd97ae5ca1501ae
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.
(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
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?
Sure Colin, please do another quick round of testing and then we can get this merged if the results still look good.
Review of attachment 221098 [details] [review]: Everything in the gnome-ostree 3.8 manifest builds.
Comment on attachment 221098 [details] [review] updated, make declaration after statement only a warning at maximum Fantastic! Pushed to master as commit e42ca9bec9d83aa53d7f79c480daf4b7c4d95fe1.
eog and anjuta (at least) are currently breaking jhbuild due to this.
brasero as well...
eog is already fixed by http://git.gnome.org/browse/eog/commit/?id=84c746b2b23e8a0b80ee9e7f18a4414707cc8002 anjuta builds OK here. brasero I just fixed: http://git.gnome.org/browse/brasero/commit/?id=5ccdd686d5e551a0b402641d5fbafb0fc8a38d07
*** Bug 568546 has been marked as a duplicate of this bug. ***
The patch at https://bugzilla.gnome.org/show_bug.cgi?id=687664 fixes the anjuta errors for me.