In gcc 8, -Wall -Wextra includes -Wcast-function-type, which warns about passing an extra (unwanted) parameter to callbacks in constructs like: _dbus_list_foreach (&my_list, (DBusForeachFunction) dbus_free, NULL); This breaks developer builds on current Debian unstable.
Created attachment 140784 [details] [review] test: Fix signature of dbus_internal_do_not_use_try_message_file In gcc 8, -Wall -Wextra includes -Wcast-function-type, which warns about casting a function pointer to an incompatible type. In this case the cast was because we were ignoring the void * argument, which in this case is NULL. Since this function is only used within dbus-message-util.c anyway, we might as well just use the correct signature and remove the cast.
Created attachment 140785 [details] [review] message-util: Make more functions static (and remove useless prefix) The naming convention dbus_internal_do_not_use_foo() was for functions that had to be exported by libdbus but called by the embedded tests. This is obsolete (in favour of _dbus_foo()) now that we have DBUS_PRIVATE_EXPORT, and is doubly useless in this case because these functions aren't even in libdbus - they're local to dbus-message-util.c.
Created attachment 140786 [details] [review] test: Don't use dbus_internal prefix for functions not in libdbus As in the previous commit, this prefix is meaningless in translation units that don't get compiled into libdbus.
Created attachment 140787 [details] [review] Rename dbus_internal_do_not_use_get_uuid to _dbus_get_uuid This was the only remaining symbol using the long prefix. Renaming it gives us one consistent rule: symbols starting with dbus are public, symbols starting with _dbus are not.
Created attachment 140788 [details] [review] Add and use _dbus_list_clear_full In gcc 8, -Wall -Wextra includes -Wcast-function-type, which warns about passing an extra (unwanted) parameter to callbacks. Instead of using _dbus_list_foreach(), add a function to do what we actually wanted here.
Created attachment 140789 [details] [review] dbus_connection_dispatch: Avoid using _dbus_list_foreach In gcc 8, -Wall -Wextra includes -Wcast-function-type, which warns about passing an extra (unwanted) parameter to callbacks. Instead of using _dbus_list_foreach(), open-code the equivalent here.
Created attachment 140790 [details] [review] test: Avoid g_queue_foreach In gcc 8, -Wall -Wextra includes -Wcast-function-type, which warns about passing an extra (unwanted) parameter to callbacks. Instead of using g_list_foreach(), open-code the equivalent. --- There's currently no g_queue_clear_full() to go with g_queue_free_full().
Created attachment 140791 [details] [review] [1.12] build: Disable new gcc 8 warning -Wcast-function-type The foreach(list, (DBusForeachFunction) free, NULL) idiom seems too entrenched to remove it from stable branches. --- dbus 1.8 and 1.10 would need this disabled in the TP_COMPILER_WARNINGS invocation instead, with different syntax but essentially the same effect.
Created attachment 140792 [details] [review] [1.10] build: Disable new gcc 8 warning -Wcast-function-type --- dbus 1.10 version of Attachment #140791 [details]. This should also work for dbus 1.8, although I haven't tried it there yet.
Created attachment 140794 [details] [review] message-util: Make more functions static (and remove useless prefix The naming convention dbus_internal_do_not_use_foo() was for functions that had to be exported by libdbus but called by the embedded tests. This is obsolete (in favour of _dbus_foo()) now that we have DBUS_PRIVATE_EXPORT, and is doubly useless in this case because these functions aren't even in libdbus - they're local to dbus-message-util.c. --- v2: Move forward declarations under the DBUS_ENABLE_EMBEDDED_TESTS conditional to avoid "declared static but not defined" warnings.
Comment on attachment 140784 [details] [review] test: Fix signature of dbus_internal_do_not_use_try_message_file Review of attachment 140784 [details] [review]: ----------------------------------------------------------------- LGTM.
Comment on attachment 140791 [details] [review] [1.12] build: Disable new gcc 8 warning -Wcast-function-type Review of attachment 140791 [details] [review]: ----------------------------------------------------------------- OK, disabling a warning can't be a build problem. LGTM.
Comment on attachment 140786 [details] [review] test: Don't use dbus_internal prefix for functions not in libdbus Review of attachment 140786 [details] [review]: ----------------------------------------------------------------- Good catch. LGTM
Comment on attachment 140787 [details] [review] Rename dbus_internal_do_not_use_get_uuid to _dbus_get_uuid Review of attachment 140787 [details] [review]: ----------------------------------------------------------------- I thought this rule applied since 2006... LGTM.
Comment on attachment 140788 [details] [review] Add and use _dbus_list_clear_full Review of attachment 140788 [details] [review]: ----------------------------------------------------------------- LGTM.
Comment on attachment 140789 [details] [review] dbus_connection_dispatch: Avoid using _dbus_list_foreach Review of attachment 140789 [details] [review]: ----------------------------------------------------------------- LGTM
Comment on attachment 140790 [details] [review] test: Avoid g_queue_foreach Review of attachment 140790 [details] [review]: ----------------------------------------------------------------- LGTM. Except for the fact that we're using inefficient linked lists, but that's not something we're likely to change.
(In reply to Thiago Macieira from comment #14) > I thought this rule applied since 2006... Sort of, but it couldn't apply completely consistently until Bug #83115, which gave us the ability to export _dbus symbols for intra-package use by dbus tools and tests (while providing a strong indication that they are not for use by external consumers, by deliberately breaking their ABI with every release).
Comment on attachment 140792 [details] [review] [1.10] build: Disable new gcc 8 warning -Wcast-function-type Review of attachment 140792 [details] [review]: ----------------------------------------------------------------- I'll apply this unreviewed unless someone wants to veto it, since it's essentially the same as the 1.12 fix, which Thiago reviewed.
Comment on attachment 140794 [details] [review] message-util: Make more functions static (and remove useless prefix Review of attachment 140794 [details] [review]: ----------------------------------------------------------------- Any opinion on this commit? (I don't think this is critical-path for fixing the warning)
Comment on attachment 140794 [details] [review] message-util: Make more functions static (and remove useless prefix Review of attachment 140794 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 140790 [details] [review] test: Avoid g_queue_foreach Review of attachment 140790 [details] [review]: ----------------------------------------------------------------- We could (in future) benefit from a g_queue_clear_full() function to be provided by GLib here. I’ve filed https://gitlab.gnome.org/GNOME/glib/issues/1464. r+ though
Comment on attachment 140792 [details] [review] [1.10] build: Disable new gcc 8 warning -Wcast-function-type Review of attachment 140792 [details] [review]: ----------------------------------------------------------------- r+ similarly
r+ for all the other patches which I haven’t explicitly commented on. Thanks for doing these thankless fixes.
Fixed in git for 1.12.0, 1.13.6 and 1.10.28, thanks
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.