GNOME Bugzilla – Bug 796341
[PATCH] gmem.h: Use typeof() in g_steal_pointer() macro
Last modified: 2018-05-23 16:53:11 UTC
Created attachment 372341 [details] [review] Patch g_steal_pointer is both an inline function, returning gpointer, and a macro that casts the return value to the type of its argument. The first version of the macro uses '0 ? (*(pp)) : (g_steal_pointer) (pp)' to cast the return value to the type of *pp, but this fails to yield warnings about incompatible pointer types with current gcc. Apparently the ternary operator is optimized away before the type of the expression is determined. The typeof() (or __typeof__()) operator allows an explicit cast.
Created attachment 372342 [details] Test case This test case shows the failure of gcc to warn about incompatible pointer types in the '0 ?...' version.
Review of attachment 372341 [details] [review]: ::: glib/gmem.h @@ +198,3 @@ /* type safety */ #define g_steal_pointer(pp) \ + ((__typeof__(*pp)) (g_steal_pointer) (pp)) We can only use __typeof__ with compilers which support it, so this needs the same safety guards as g_object_ref(): #if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8)) && !defined(__cplusplus) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_56 In future, we might want to factor that out into a test in configure.ac/meson.build, but no need to do that in this bug report unless you really want to (if so, do it as a separate patch; it would be appreciated!).
Is the GLIB_VERSION_MAX_ALLOWED part needed here? As I recall, it was a transition thing to help cope with the API change in g_object_ref(), but there isn't one here.
(In reply to Peter Bloomfield from comment #3) > Is the GLIB_VERSION_MAX_ALLOWED part needed here? As I recall, it was a > transition thing to help cope with the API change in g_object_ref(), but > there isn't one here. Yes, it is needed (though obviously the version should be changed to 2.58), because this could cause compile errors in code using GLib with -Werror, where the stolen pointer is assigned to a variable which has a different type from the variable being stolen from. For example, a supertype or subtype. Code would need modifying to add an explicit cast, which means this would be an API break. Adding the GLIB_VERSION_MAX_ALLOWED check is a way of opting in to that API break. See bug #790697.
Created attachment 372366 [details] [review] Revised patch Thanks for the review! Revised patch attached.
Review of attachment 372366 [details] [review]: Great, thanks
Pushed to master, thanks.