Bug 107349

Summary: Fix build with gcc 8 -Werror=cast-function-type
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: 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
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.
Comment 1 Simon McVittie 2018-07-23 17:09:50 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.
Comment 2 Simon McVittie 2018-07-23 17:10:16 UTC
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.
Comment 3 Simon McVittie 2018-07-23 17:10:39 UTC
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.
Comment 4 Simon McVittie 2018-07-23 17:11:08 UTC
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.
Comment 5 Simon McVittie 2018-07-23 17:11:34 UTC
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.
Comment 6 Simon McVittie 2018-07-23 17:12:02 UTC
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.
Comment 7 Simon McVittie 2018-07-23 17:12:50 UTC
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().
Comment 8 Simon McVittie 2018-07-23 17:23:31 UTC
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.
Comment 9 Simon McVittie 2018-07-23 17:53:18 UTC
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.
Comment 10 Simon McVittie 2018-07-23 18:08:23 UTC
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 11 Thiago Macieira 2018-07-28 21:13:27 UTC
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 12 Thiago Macieira 2018-07-28 21:15:10 UTC
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 13 Thiago Macieira 2018-07-28 21:16:13 UTC
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 14 Thiago Macieira 2018-07-28 21:16:45 UTC
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 15 Thiago Macieira 2018-07-28 21:27:17 UTC
Comment on attachment 140788 [details] [review]
Add and use _dbus_list_clear_full

Review of attachment 140788 [details] [review]:
-----------------------------------------------------------------

LGTM.
Comment 16 Thiago Macieira 2018-07-28 21:37:05 UTC
Comment on attachment 140789 [details] [review]
dbus_connection_dispatch: Avoid using _dbus_list_foreach

Review of attachment 140789 [details] [review]:
-----------------------------------------------------------------

LGTM
Comment 17 Thiago Macieira 2018-07-28 21:38:10 UTC
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.
Comment 18 Simon McVittie 2018-07-29 10:55:24 UTC
(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 19 Simon McVittie 2018-07-30 10:53:25 UTC
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 20 Simon McVittie 2018-07-30 10:54:38 UTC
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 21 Philip Withnall 2018-07-30 14:33:04 UTC
Comment on attachment 140794 [details] [review]
message-util: Make more functions static (and remove  useless prefix

Review of attachment 140794 [details] [review]:
-----------------------------------------------------------------

r+
Comment 22 Philip Withnall 2018-07-30 14:48:04 UTC
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 23 Philip Withnall 2018-07-30 14:49:12 UTC
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
Comment 24 Philip Withnall 2018-07-30 14:49:38 UTC
r+ for all the other patches which I haven’t explicitly commented on. Thanks for doing these thankless fixes.
Comment 25 Simon McVittie 2018-08-02 22:13:32 UTC
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.