GNOME Bugzilla – Bug 793399
Fix some cases of -Wduplicated-branches
Last modified: 2018-05-17 17:39:55 UTC
The checks that the argument passed is a pointer to an integer contain duplicate branches, which GCC complains about very loudly as of version 7.
Created attachment 368275 [details] [review] gatomic: fix -Wduplicated-branches
Review of attachment 368275 [details] [review]: GCC 7.3.1 now gives the following warnings for me for all of the changed lines: ../../source/glib/gio/gresource.c: In function ‘g_static_resource_fini’: ../../source/glib/glib/gatomic.h:117:38: warning: pointer/integer type mismatch in conditional expression (void) (0 ? (gpointer) *(atomic) : 1); \ ^
Created attachment 368323 [details] [review] gatomic: fix -Wduplicated-branches The checks that the argument passed is a pointer to an integer contain duplicate branches, which GCC complains about very loudly as of version 7.
Created attachment 368324 [details] [review] gtester: fix -Wduplicated-branches The only thing to worry about now is an even more obscure warning complaining about tautological comparisons.
(In reply to Philip Withnall from comment #2) > Review of attachment 368275 [details] [review] [review]: > > GCC 7.3.1 now gives the following warnings for me for all of the changed > lines: > > ../../source/glib/gio/gresource.c: In function ‘g_static_resource_fini’: > ../../source/glib/glib/gatomic.h:117:38: warning: pointer/integer type > mismatch in conditional expression > (void) (0 ? (gpointer) *(atomic) : 1); > \ > ^ Forgot that I was building glib with Clang. It should only be half-ish of the changed lines that cause this.
(In reply to Ernestas Kulik from comment #4) > Created attachment 368324 [details] [review] [review] > gtester: fix -Wduplicated-branches > > The only thing to worry about now is an even more obscure warning > complaining about tautological comparisons. Please tell me if this makes sense or if a comment would be better.
Review of attachment 368324 [details] [review]: ::: glib/gtester.c @@ +323,3 @@ if (!subtest_mode_fatal) argc++; + if (subtest_mode_quick || !subtest_mode_quick) That’s unclear and will probably cause compiler warnings in future about tautological comparisons. As you suggest, I think that removing the branch and adding a comment. Something like: ``` /* We always append either -m=quick or -m=slow: */ argc++; ```
Created attachment 368427 [details] [review] gtester: fix -Wduplicated-branches One of the “quick” or “slow” test run modes is always added to the argument list, making the branching pointless, which, coincidentally, now causes a warning.
(In reply to Philip Withnall from comment #7) > Review of attachment 368324 [details] [review] [review]: > > ::: glib/gtester.c > @@ +323,3 @@ > if (!subtest_mode_fatal) > argc++; > + if (subtest_mode_quick || !subtest_mode_quick) > > That’s unclear and will probably cause compiler warnings in future about > tautological comparisons. As you suggest, I think that removing the branch > and adding a comment. Something like: > > ``` > /* We always append either -m=quick or -m=slow: */ > argc++; > ``` Welp, I completely missed the code below that.
Review of attachment 368323 [details] [review]: > The checks that the argument passed is a pointer to an integer contain > duplicate branches, which GCC complains about very loudly as of version > 7. I thought you were testing with GCC 8, not 7? Please amend the commit message. Aside from that, this looks good. Could you submit a third patch which adds -Wduplicated-branches to the set of warnings we enable by default in configure.ac (https://gitlab.gnome.org/GNOME/glib/blob/master/configure.ac#L3481) and meson.build (code doesn’t currently exist in GLib; copy https://gitlab.gnome.org/GNOME/gtk/blob/master/meson.build#L214 and change it to use the set of warnings from configure.ac + -Wduplicated-branches) please?
Review of attachment 368427 [details] [review]: Thanks. I’ll push this to master and glib-2-54.
Comment on attachment 368427 [details] [review] gtester: fix -Wduplicated-branches Pushed to master. Attachment 368427 [details] pushed as c97922c - gtester: fix -Wduplicated-branches
(In reply to Philip Withnall from comment #10) > Review of attachment 368323 [details] [review] [review]: > > > The checks that the argument passed is a pointer to an integer contain > > duplicate branches, which GCC complains about very loudly as of version > > 7. > > I thought you were testing with GCC 8, not 7? Please amend the commit > message. > > Aside from that, this looks good. Could you submit a third patch which adds > -Wduplicated-branches to the set of warnings we enable by default in > configure.ac > (https://gitlab.gnome.org/GNOME/glib/blob/master/configure.ac#L3481) and > meson.build (code doesn’t currently exist in GLib; copy > https://gitlab.gnome.org/GNOME/gtk/blob/master/meson.build#L214 and change > it to use the set of warnings from configure.ac + -Wduplicated-branches) > please? The way I opened might have been misleading. The context is that Continuous fails to build gnome-desktop, because it was recently switched to use AX_COMPILER_FLAGS and that implies -Werror.
And pushed to glib-2-54 too. commit fa033ad638d20870412e646e6ccee212b11b3210 (HEAD -> glib-2-54, origin/glib-2-54) Author: Ernestas Kulik <ernestask@gnome.org> Date: Tue Feb 13 20:57:04 2018 +0200 gtester: fix -Wduplicated-branches One of the “quick” or “slow” test run modes is always added to the argument list, making the branching pointless, which, coincidentally, now causes a warning. https://bugzilla.gnome.org/show_bug.cgi?id=793399 glib/gtester.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
(In reply to Ernestas Kulik from comment #13) > The way I opened might have been misleading. The context is that Continuous > fails to build gnome-desktop, because it was recently switched to use > AX_COMPILER_FLAGS and that implies -Werror. Sure, but if we want GLib to be usable from modules which compile with -Wduplicated-branches, we need to be able to test that — or we’ll get regressions on this in future. The easiest way to test that on an ongoing basis is to unconditionally enable that warning when building GLib. I’ve just compiled GLib with -Wduplicated-branches (GCC 7.3.1) and the only other warning comes from data-input-stream.c — so this would be fairly easy to do, if you wouldn’t mind.
(In reply to Philip Withnall from comment #15) > (In reply to Ernestas Kulik from comment #13) > > The way I opened might have been misleading. The context is that Continuous > > fails to build gnome-desktop, because it was recently switched to use > > AX_COMPILER_FLAGS and that implies -Werror. > > Sure, but if we want GLib to be usable from modules which compile with > -Wduplicated-branches, we need to be able to test that — or we’ll get > regressions on this in future. The easiest way to test that on an ongoing > basis is to unconditionally enable that warning when building GLib. > > I’ve just compiled GLib with -Wduplicated-branches (GCC 7.3.1) and the only > other warning comes from data-input-stream.c — so this would be fairly easy > to do, if you wouldn’t mind. I was reacting to you thinking that I was testing with GCC 8. I also have 7.3.1 installed. Reading the 7 series release notes, it seems that that’s when it was added. I don’t mind adding that warning or fixing other cases. :)
(In reply to Ernestas Kulik from comment #16) > (In reply to Philip Withnall from comment #15) > > (In reply to Ernestas Kulik from comment #13) > > > The way I opened might have been misleading. The context is that Continuous > > > fails to build gnome-desktop, because it was recently switched to use > > > AX_COMPILER_FLAGS and that implies -Werror. > > > > Sure, but if we want GLib to be usable from modules which compile with > > -Wduplicated-branches, we need to be able to test that — or we’ll get > > regressions on this in future. The easiest way to test that on an ongoing > > basis is to unconditionally enable that warning when building GLib. > > > > I’ve just compiled GLib with -Wduplicated-branches (GCC 7.3.1) and the only > > other warning comes from data-input-stream.c — so this would be fairly easy > > to do, if you wouldn’t mind. > > I was reacting to you thinking that I was testing with GCC 8. I also have > 7.3.1 installed. Reading the 7 series release notes, it seems that that’s > when it was added. Ah, apologies, I was assuming this was related to bug #793272 (which is only with GCC 8), which was filed at roughly the same time. > I don’t mind adding that warning or fixing other cases. :) That would be very helpful, thanks! I’ll merge your gatomic patch and wait for the remaining ones.
Review of attachment 368323 [details] [review]: I’ll push this to master and glib-2-54 shortly.
Comment on attachment 368323 [details] [review] gatomic: fix -Wduplicated-branches Attachment 368323 [details] pushed as 62e8168 - gatomic: fix -Wduplicated-branches
Pushed to glib-2-54 as well. commit 76fdf67ae598baf4a6bc38e79b5ba514c428ebf0 (HEAD -> glib-2-54, origin/glib-2-54) Author: Ernestas Kulik <ernestask@gnome.org> Date: Mon Feb 12 20:20:57 2018 +0200 gatomic: fix -Wduplicated-branches The checks that the argument passed is a pointer to an integer contain duplicate branches, which GCC complains about very loudly as of version 7. https://bugzilla.gnome.org/show_bug.cgi?id=793399 glib/gatomic.h | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)
Created attachment 368435 [details] [review] tests: data-input-stream: fix -Wduplicated-branches The fix makes things a bit awkward, but it seems to work just fine.
Created attachment 368436 [details] [review] configure.ac: add -Wduplicated-branches
Created attachment 368437 [details] [review] build: meson: add warning flags This commit copies the code from GTK+ to add warning flags to the same targets as in the Autotools build.
Created attachment 368438 [details] [review] tests: data-input-stream: fix -Wduplicated-branches The fix makes things a bit awkward, but it seems to work just fine.
Review of attachment 368438 [details] [review]: OK.
Review of attachment 368436 [details] [review]: Yup, thanks.
Review of attachment 368437 [details] [review]: There are 19 meson.build files in GLib which mention `c_args : blah`; I think all of them need to be patched to use common_c_args consistently. ::: gio/meson.build @@ +790,3 @@ libgobject_dep, libgmodule_dep, selinux_dep, xattr_dep, platform_deps, network_libs], + c_args : [common_c_args, gio_c_args], Shouldn’t that be a `+` rather than a list of lists? ::: gio/tests/meson.build @@ +81,3 @@ '-DGLIB_MKENUMS="@0@"'.format(glib_mkenums), '-DGLIB_COMPILE_SCHEMAS="@0@"'.format(glib_compile_schemas.full_path()), + common_c_args, Shouldn’t that be `] + common_c_args`? ::: glib/tests/meson.build @@ +100,3 @@ + '-DHAVE_CONFIG_H=1', + '-DG_LOG_DOMAIN="GLib"', + common_c_args, Shouldn’t that be `] + common_c_args`? ::: gobject/tests/meson.build @@ +39,3 @@ endif exe = executable(test_name, test_src, + c_args : ['-DHAVE_CONFIG_H=1', '-DG_LOG_DOMAIN="GLib-GObject"', common_c_args], Shouldn’t that be `] + common_c_args`? ::: meson.build @@ +275,3 @@ + # in GLib, based on _Win32_Programming_ by Rector and Newcomer + test_c_args = [] + add_project_arguments('-FImsvc_recommended_pragmas.h', language: 'c') This .h file is already included above, so no need to include it here. @@ +276,3 @@ + test_c_args = [] + add_project_arguments('-FImsvc_recommended_pragmas.h', language: 'c') + add_project_arguments('-D_USE_MATH_DEFINES', language: 'c') `configure.ac` doesn’t do this, so I’d rather not add it here. @@ +316,3 @@ + '-Werror=pointer-to-int-cast', + '-Werror=empty-body', + '-Werror=write-strings', Have you tested that GLib compiles with all these?
Comment on attachment 368438 [details] [review] tests: data-input-stream: fix -Wduplicated-branches Pushed the tests fix to master, and will shortly push it to glib-2-54 too. Attachment 368438 [details] pushed as c01c255 - tests: data-input-stream: fix -Wduplicated-branches
(In reply to Philip Withnall from comment #27) > Review of attachment 368437 [details] [review] [review]: > > There are 19 meson.build files in GLib which mention `c_args : blah`; I > think all of them need to be patched to use common_c_args consistently. Sure. I was going for that originally, but changed my mind halfway through. > ::: gio/meson.build > @@ +790,3 @@ > libgobject_dep, libgmodule_dep, selinux_dep, xattr_dep, > platform_deps, network_libs], > + c_args : [common_c_args, gio_c_args], > > Shouldn’t that be a `+` rather than a list of lists? Since some recent-ish version of Meson, nested lists are flattened, so it’s more or less a matter of taste. I’d like this to be consistently lists everywhere, but I’m not going to put up a fight for it. > ::: meson.build > @@ +275,3 @@ > + # in GLib, based on _Win32_Programming_ by Rector and Newcomer > + test_c_args = [] > + add_project_arguments('-FImsvc_recommended_pragmas.h', language: 'c') > > This .h file is already included above, so no need to include it here. Sorry, some cowboy pasting there. > @@ +276,3 @@ > + test_c_args = [] > + add_project_arguments('-FImsvc_recommended_pragmas.h', language: 'c') > + add_project_arguments('-D_USE_MATH_DEFINES', language: 'c') > > `configure.ac` doesn’t do this, so I’d rather not add it here. Okay. > @@ +316,3 @@ > + '-Werror=pointer-to-int-cast', > + '-Werror=empty-body', > + '-Werror=write-strings', > > Have you tested that GLib compiles with all these? It built fine on my machine, I can only say that much.
Created attachment 368441 [details] [review] build: meson: add warning flags Courtesy of GTK+.
Reused the same warnings as in the Autotools definitions, added the flags to all files, removed unneeded things and used the + operator for list concatenation. Makes me wonder if the warning arguments shouldn’t be added globally as project arguments, instead of dancing around like this.
(In reply to Ernestas Kulik from comment #31) > Makes me wonder if the warning arguments shouldn’t be added globally as > project arguments, instead of dancing around like this. Yeah. Emmanuele, what’s the recommended approach here? Why did GTK+ do it the way it did?
Mainly historical reasons: we didn't want to use the same flags for everything. We can be strict about GTK itself, but tests are allowed to be less strict because they can test either real-world code, or edge cases. Additionally, some tests did trip compiler warnings, but never got fixed, so it was easier to be lax.
(In reply to Philip Withnall from comment #32) > Why did GTK+ do it the way it did? The Meson maintainer feels strongly that the AX_COMPILER_FLAGS approach is the wrong one, and individual projects should enable the warnings that they (care about|have been build-tested against).
Review of attachment 368441 [details] [review]: I think this looks okay; you could replace the "add common_c_args everywhere" with `add_project_arguments()` after testing what's available, assuming the build passes. ;-) Ideally, though, I'd like to see another common on top of this that removes the pointless `HAVE_CONFIG_H` define and wherever we check for it (we always have a "config.h", and we cannot build GLib source files outside of GLib; this is not gnulib). ::: meson.build @@ +290,3 @@ + +common_c_args = [] +foreach cflag: test_c_args No need to do this any more; Meson 0.43 added `get_supported_arguments()` to the compiler object, so you can turn this into: ``` common_c_args = cc.get_supported_arguments(test_c_args) ```
(In reply to Emmanuele Bassi (:ebassi) from comment #35) > Review of attachment 368441 [details] [review] [review]: > > Ideally, though, I'd like to see another common on top of this that removes > the pointless `HAVE_CONFIG_H` define and wherever we check for it (we always > have a "config.h", and we cannot build GLib source files outside of GLib; > this is not gnulib). Does that include gio/gdbus-2.0/codegen/codegen.py and po/po2tbl.sed.in?
(In reply to Ernestas Kulik from comment #36) > (In reply to Emmanuele Bassi (:ebassi) from comment #35) > > Review of attachment 368441 [details] [review] [review] [review]: > > > > Ideally, though, I'd like to see another common on top of this that removes > > the pointless `HAVE_CONFIG_H` define and wherever we check for it (we always > > have a "config.h", and we cannot build GLib source files outside of GLib; > > this is not gnulib). > > Does that include gio/gdbus-2.0/codegen/codegen.py and po/po2tbl.sed.in? Leave it in codegen.py (since we’re generating code for other projects, who may care about an optional config.h); and leave po2tbl.sed.in alone too, since that’s copy-pasted from elsewhere, so keeping the delta small is preferable.
Created attachment 368648 [details] [review] build: meson: add warning flags Courtesy of GTK+.
Created attachment 368649 [details] [review] Remove HAVE_CONFIG_H defs and uses
Created attachment 368650 [details] [review] gio: tests: modules: declare _get_type prototype GCC complains about non-static functions being defined without a previous prototype, even if they themselves provide one.
Review of attachment 368650 [details] [review]: Sure. I’ll backport this to glib-2-56 too.
Review of attachment 368649 [details] [review]: OK. I suspect we could drop HAVE_CONFIG_H from all the build/win32/ and win32/vs*/ files too.
Review of attachment 368648 [details] [review]: Looks good, thanks.
(In reply to Philip Withnall from comment #42) > Review of attachment 368649 [details] [review] [review]: > > OK. I suspect we could drop HAVE_CONFIG_H from all the build/win32/ and > win32/vs*/ files too. I didn’t know how those are created, so I avoided touching them, but it seems harmless enough.
Comment on attachment 368650 [details] [review] gio: tests: modules: declare _get_type prototype Attachment 368650 [details] pushed as 4f94212 - gio: tests: modules: declare _get_type prototype
(In reply to Philip Withnall from comment #41) > Review of attachment 368650 [details] [review] [review]: > > Sure. I’ll backport this to glib-2-56 too. I meant glib-2-54. Pushed: commit d75a6aeb4f07512d4154729db09d1310a58840b1 (HEAD -> glib-2-54, origin/glib-2-54) Author: Ernestas Kulik <ernestask@gnome.org> Date: Mon Feb 19 16:14:06 2018 +0200 gio: tests: modules: declare _get_type prototype GCC complains about non-static functions being defined without a previous prototype, even if they themselves provide one. https://bugzilla.gnome.org/show_bug.cgi?id=793399 gio/tests/modules/test-module-a.c | 2 ++ gio/tests/modules/test-module-b.c | 2 ++ 2 files changed, 4 insertions(+)
Created attachment 368711 [details] [review] Remove HAVE_CONFIG_H defs and uses Since GLib files are only meant to be built as part of GLib, config.h always exists, so the checks are more or less pointless.
Review of attachment 368711 [details] [review]: Thanks, I’ll push these all shortly.
Three remaining patches pushed to master. They do not need to be pushed to glib-2-54. Thanks for your work on this! Attachment 368436 [details] pushed as aef0e69 - configure.ac: add -Wduplicated-branches Attachment 368648 [details] pushed as 9d24c8b - build: meson: add warning flags Attachment 368711 [details] pushed as 03e86d0 - Remove HAVE_CONFIG_H defs and uses
-Wdeclaration-after-statement breaks build on macos: ../gio/kqueue/dep-list.c:91:15: error: ISO C90 forbids mixing declarations and code [-Werror,-Wdeclaration-after-statement]
Oops, sorry, checked git blame too fast. Using autotools it won't build kqueue with those flags, it's meson who add those flags project-wide.