| Summary: | Fix build with gcc 8 -Werror=cast-function-type | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | Keywords: | patch |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | review+ | ||
| i915 platform: | i915 features: | ||
| Attachments: |
test: Fix signature of dbus_internal_do_not_use_try_message_file
message-util: Make more functions static (and remove useless prefix) test: Don't use dbus_internal prefix for functions not in libdbus Rename dbus_internal_do_not_use_get_uuid to _dbus_get_uuid Add and use _dbus_list_clear_full dbus_connection_dispatch: Avoid using _dbus_list_foreach test: Avoid g_queue_foreach [1.12] build: Disable new gcc 8 warning -Wcast-function-type [1.10] build: Disable new gcc 8 warning -Wcast-function-type message-util: Make more functions static (and remove useless prefix |
||
|
Description
Simon McVittie
2018-07-23 17:08:58 UTC
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.