This could be solved by wrapping the cleanup func in another static function, like in https://paste.xinu.at/zYR7Vn/ .

Would this be acceptable?

(In reply to Jan Alexander Steffens (heftig) from comment #1)
> This could be solved by wrapping the cleanup func in another static
> function, like in https://paste.xinu.at/zYR7Vn/ .
> 
> Would this be acceptable?

Yeah, that would be acceptable. Have you tested that it will still warn if passed an incorrect pointer type? e.g.

static void my_struct_free (SomethingOtherThanMyStruct *blah);
G_DEFINE_AUTOPTR_CLEANUP_FUNC (MyStruct, my_struct_free)

If so, please attach a git formatted patch and we can get this fixed quickly. Thanks.

To summarize, the options I see are:

1. Have downstream use a static inline wrapper, like attachment 372178 [details] [review].
- Reduces g_auto(s)list performance by introducing a static trampoline function.
- Might reduce g_autoptr performance depending on inlining optimization.

2. Have downstream turn off the warning, like attachment 372034 [details] [review].
- Quite a bit of compiler-specific boilerplate.
- Might turn off a relevant warning, but only locally.

3. Always wrap the function in g_auto(s)list, like comment 1.
- Reduces g_auto(s)list performance even if the wrapper isn't needed.

4. Always turn off the warning around g_auto(s)list.
- Turns off the warning even in the cases where it could be relevant.
- Hard to discover that it does because it's hidden behind a macro.

Created attachment 372334 [details] [review]
macros: Wrap cleanup func for g_autolist; avoids cast warning

For g_autolist and g_autoslist, the cleanup func was cast to
GDestroyNotify before being passed to g_(s)list_free_full. This cast
provokes GCC 8 to emit a warning if the return type is not void:

  …/gmacros.h:462:99: warning: cast between incompatible function types
  from … to 'void (*)(void *)' [-Wcast-function-type]

Avoid the warning by replacing the cast with a wrapper function that is
a static inline GDestroyNotify. g_autoptr remains untouched.

The patch implements (3.) above.

(In reply to Philip Withnall from comment #2)
> Yeah, that would be acceptable. Have you tested that it will still warn if passed an incorrect pointer type?

The patch I just attached doesn't touch the g_autoptr path so you still get the warnings for a bad argument type that you did before. The only improvement I can see is replacing the implementation of the wrapper func with { (func) ((TypeName *) _ptr); } but I think that would just duplicate the warning you already get from the autoptr func.

Review of attachment 372334 [details] [review]:

(In reply to Jan Alexander Steffens (heftig) from comment #3)
> 3. Always wrap the function in g_auto(s)list, like comment 1.
> - Reduces g_auto(s)list performance even if the wrapper isn't needed.

Would it? All of those functions are static inline, so the compiler should be able to inline everything together and optimise the call chain.

Could you please add a unit test for this? i.e. One which passes a cleanup function which has a non-void return type. The unit test should compile without warnings (but no need to modify the build system to pass -Werror, because that’s too much faff).

Review of attachment 372334 [details] [review]:

::: glib/gmacros.h
@@ +485,3 @@
   G_GNUC_BEGIN_IGNORE_DEPRECATIONS                                                                              \
   static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) { if (*_ptr) (func) (*_ptr); }         \
+  static inline void _GLIB_AUTOPTR_WRAP_FUNC_NAME(TypeName) (gpointer _ptr) { (func) (_ptr); }                  \

Also, as per bug #796346, this should have G_GNUC_UNUSED (sorry, I got to that bug after this one!).

(In reply to Philip Withnall from comment #6)
> Would it? All of those functions are static inline, so the compiler should be able to inline everything together and optimise the call chain.
g_(s)list_free_full is not inlined so the compiler has no choice but to pass the address of the wrapper function.
I'm pretty sure it won't optimize this to the address of the wrapped function. So that's an additional PLT/GOT call for each list element.


(In reply to Philip Withnall from comment #7)
The wrap function is being used by the list and slist functions, though, so shouldn't that avoid an unused function warning?

Actually, the GCC documentation recommends a double-cast so that looks like the better solution.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wcast-function-type

Created attachment 372360 [details] [review]
macros: Double-cast func for g_autolist to avoid warning

For g_autolist and g_autoslist, the cleanup func was cast to
GDestroyNotify before being passed to g_(s)list_free_full. This cast
provokes GCC 8 to emit a warning if the return type is not void:

  …/gmacros.h:462:99: warning: cast between incompatible function types
  from … to 'void (*)(void *)' [-Wcast-function-type]

Cast to 'void (*)(void)' first, which suppresses the warning as
recommended by the GCC documentation. g_autoptr remains untouched.

Created attachment 372361 [details] [review]
macros: Double-cast func for g_autolist to avoid warning

Forgot meson.

Review of attachment 372361 [details] [review]:

I was concerned about the fact that we’d completely lose the type checking by doing the (void(*)(void)) cast, but we actually still get type checking from the _GLIB_AUTOPTR_FUNC_NAME function, so the changes to gmacros.h are fine here.

::: glib/tests/Makefile.am
@@ +225,3 @@
 	$(NULL)
+
+autoptr_CFLAGS = $(AM_CFLAGS) -Wextra -Wno-unused-parameter

Unfortunately we can’t just pass these flags to the compiler without first checking that the compiler supports them. grep for '-W' in meson.build or configure.ac to see the checks we already do for general-purpose compiler warnings.

If you really want to add support for testing for these particular flags in configure.ac/meson.build, please do; but it’s not something I was expecting to be done for fixing this bug, since it’s a pain to write and test for not much benefit. I’d be tempted to keep the changes to autoptr.c but drop the Makefile.am/meson.build changes.

There's already the 'atomic' test, which is also guarded by HAVE_GCC and adds the flag -Wstrict-aliasing=2 without checking it's supported.

*** Bug 796087 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1382.