After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 449565 - Add G_DEFINE_BOXED_TYPE()
Add G_DEFINE_BOXED_TYPE()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Tim Janik
gtkdev
Depends on:
Blocks: 539622 562201 578018
 
 
Reported: 2007-06-20 19:00 UTC by Behdad Esfahbod
Modified: 2011-03-22 14:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add G_DEFINE_[BOXED|POINTER]_TYPE, generating threadsafe get_type functions; and use them in gobject (6.82 KB, patch)
2007-12-26 00:53 UTC, Christian Persch
none Details | Review
updated patch; document the new macros (3.55 KB, patch)
2008-01-07 12:43 UTC, Christian Persch
none Details | Review
[PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565. (4.89 KB, patch)
2008-05-31 18:21 UTC, Christian Persch
none Details | Review
[PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565. (4.27 KB, patch)
2008-06-22 17:23 UTC, Christian Persch
none Details | Review
updated patch to master (4.83 KB, patch)
2009-12-01 17:16 UTC, Christian Persch
none Details | Review
updated patches (13.30 KB, patch)
2010-04-06 12:02 UTC, Christian Persch
none Details | Review
Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE] (5.82 KB, patch)
2010-07-01 18:22 UTC, Christian Persch
none Details | Review
Use G_DEFINE_[BOXED|POINTER]_TYPE instead of handwritten code (13.47 KB, patch)
2010-07-01 18:24 UTC, Christian Persch
none Details | Review
Test case (1.88 KB, text/plain)
2011-03-22 10:19 UTC, Christophe Fergeau
  Details

Description Behdad Esfahbod 2007-06-20 19:00:53 UTC
There are over 10 functions in Pango very similar to this:


GType
pango_font_metrics_get_type (void)
{ 
  static GType our_type = 0;

  if (our_type == 0)
    our_type = g_boxed_type_register_static (I_("PangoFontMetrics"),
                                             (GBoxedCopyFunc)pango_font_metrics_ref,
                                             (GBoxedFreeFunc)pango_font_metrics_unref);

  return our_type;
}

I just went over all of them and added G_UNLIKELY to them all.

(from my reading of gboxed.c, g_boxed_type_register_static() is not idempotent.  At least it's not obvious that it is.  So if we may need to change the idiom later, etc, it helps to have a macro we can adjust.)
Comment 1 Matthias Clasen 2007-06-21 05:48:35 UTC
g_boxed_type_register_static() has

  g_return_val_if_fail (g_type_from_name (name) == 0, 0);

which makes it a programming error to call it twice.
Comment 2 Tim Janik 2007-06-29 14:15:40 UTC
i'm not sure i understand exactly what is proposed here, can you please attach a patch if this worth to be kept open?
Comment 3 Behdad Esfahbod 2007-06-29 20:11:15 UTC
#define G_DEFINE_BOXED_TYPE(TypeName, type_name, _copyfunc_, _freefunc_) \
GType \
type_name##_get_type (void) \
{ \
  static GType our_type = 0; \
\
  /* TODO: make this thread-safe */ \
  if (G_UNLIKELY (our_type == 0)) \
    our_type = g_boxed_type_register_static (I_(TypeName), \
                                             (GBoxedCopyFunc)_copyfunc_, \
                                             (GBoxedFreeFunc)_freefunc_); \
\
  return our_type; \
}
Comment 4 Mathias Hasselmann (IRC: tbf) 2007-07-31 20:59:34 UTC
A more complete set of macros can be found in that file:

http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?revision=819&view=markup

It also introduces G_DEFINE_QUARK (actually EGG_DEFINE_QUARK). Guess having G_DEFINE_QUARK could improve code quality of glib applications: At least I find myself to use quarks, as I do not want to write the (short) boilerplate code needed to introduce some quark.
Comment 5 Tim Janik 2007-08-15 08:59:34 UTC
(In reply to comment #4)
> A more complete set of macros can be found in that file:
> 
> http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?revision=819&view=markup
> 
> It also introduces G_DEFINE_QUARK (actually EGG_DEFINE_QUARK). Guess having
> G_DEFINE_QUARK could improve code quality of glib applications: At least I find
> myself to use quarks, as I do not want to write the (short) boilerplate code
> needed to introduce some quark.

you should really make your _get_type (and quark registration) functions thread-safe in there. take a look at initializer1() in http://svn.gnome.org/viewcvs/glib/trunk/tests/onceinit.c?view=markup for a simple example.
Comment 6 Mathias Hasselmann (IRC: tbf) 2007-08-15 19:35:24 UTC
(In reply to comment #5)

Thread safety applied: http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?r1=823&r2=825

Shall I prepare a patch to move those macros into glib?
Comment 7 Tim Janik 2007-08-16 11:26:16 UTC
(In reply to comment #6)
> (In reply to comment #5)
> 
> Thread safety applied:
> http://svn.gnome.org/viewcvs/libegg/trunk/libegg/util/egg-macros.h?r1=823&r2=825
> 
> Shall I prepare a patch to move those macros into glib?

well, for G_DEFINE_BOXED_TYPE() that might make sense. i'm not so sure about your quark thingy though, haven't really seen the need for that yet. also, take your time on this one, since we're currently API frozen.
Comment 8 Mathias Hasselmann (IRC: tbf) 2007-08-16 13:17:41 UTC
(In reply to comment #7)
> i'm not so sure about your quark thingy though, haven't really seen the need for that yet. 

Well, you need quarks to define custom error domains and often enough I voted against GError based error reporting because I was to lazy to paste the required custom_error_quark function. Maybe a problem of mine. Don't know.
Comment 9 Behdad Esfahbod 2007-08-16 17:40:43 UTC
I find  the quark macro useful too.  Specially if using the quark in more than one function...
Comment 10 Christian Persch 2007-12-26 00:53:59 UTC
Created attachment 101601 [details] [review]
add G_DEFINE_[BOXED|POINTER]_TYPE, generating threadsafe get_type functions; and use them in gobject
Comment 11 Behdad Esfahbod 2007-12-26 03:44:00 UTC
At GUADEC Tim commented on my code that the cast to GBoxedCopyFunc and GBoxedFreeFunc should be left to the user.  So, that's one place to fix in the patch.  Other than that, looks good to me, and definitely an improvement over current practice.
Comment 12 Christian Persch 2007-12-26 13:25:18 UTC
If the copy and free funcs are public (which they usually will be), they'll take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient...
Comment 13 Christian Persch 2008-01-07 12:43:35 UTC
Created attachment 102318 [details] [review]
updated patch; document the new macros

It would be really great if this made it into 2.16 ...
Comment 14 Tommi Komulainen 2008-04-14 06:07:17 UTC
Comment on attachment 102318 [details] [review]
updated patch; document the new macros

>+/* convenience macros for boxed type registration:
>+ * macro arguments: TypeName, copy_func, free_func
>+ * example: G_DEFINE_BOXED_TYPE (GtkFoo, gtk_foo, gtk_foo_copy, gtk_foo_free)

Since this is a convenience macro, wouldn't it be more convenient to implicitly form the copy/free function names (type_name##_copy, similar to type_name##_class_init) and in case their naming is different provide G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given?
Comment 15 Yevgen Muntyan 2008-04-14 06:45:40 UTC
(In reply to comment #14)
> (From update of attachment 102318 [details] [review] [edit])
> >+/* convenience macros for boxed type registration:
> >+ * macro arguments: TypeName, copy_func, free_func
> >+ * example: G_DEFINE_BOXED_TYPE (GtkFoo, gtk_foo, gtk_foo_copy, gtk_foo_free)
> 
> Since this is a convenience macro, wouldn't it be more convenient to implicitly
> form the copy/free function names (type_name##_copy, similar to
> type_name##_class_init) and in case their naming is different provide
> G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given?

Half of types use foo_ref, half use foo_copy, and you do use those functions in real code, so the names must be meaningful and not auto-generated (unlike type_name##_class_init which you don't care about).
Comment 16 Sebastian Dröge (slomo) 2008-05-08 10:52:26 UTC
Any news on this? Would be nice to have this in 2.18 at least...
Comment 17 Sebastian Dröge (slomo) 2008-05-08 18:26:50 UTC
Another thing, apart from G_DEFINE_{POINTER,BOXED}_TYPE and G_DEFINE_QUARK would be G_DEFINE_ENUM_TYPE.

#define G_DEFINE_ENUM_TYPE(TypeName,type_name,values) \
GType \
type_name##_get_type (void) \
{ \
  static volatile gsize g_define_type_id__volatile = 0; \
  if (g_once_init_enter (&g_define_type_id__volatile))  \
    { \
      static const GEnumValue v[] = {
        values,
        {0, NULL, NULL}
      };
      GType g_define_type_id = \
         g_enum_register_static (g_intern_static_string (#TypeName), \
                                      v); \
      g_once_init_leave (&g_define_type_id__volatile, g_define_type_id); \
    }					\
  return g_define_type_id__volatile;	\
} /* closes type_name##_get_type() */
Comment 18 Olivier Crête 2008-05-08 18:41:08 UTC
if we need G_DEFINE_ENUM_TYPE, we also need G_DEFINE_FLAGS_TYPE. And support for both should also be added to glib-mkenums too if we need that
Comment 19 Christian Persch 2008-05-31 18:21:58 UTC
Created attachment 111863 [details] [review]
[PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565.

 docs/reference/gobject/gobject-sections.txt |    4 ++
 docs/reference/gobject/tmpl/gtype.sgml      |   54 +++++++++++++++++++++++++++
 gobject/gtype.h                             |   37 ++++++++++++++++++
 3 files changed, 95 insertions(+), 0 deletions(-)
Comment 20 Christian Persch 2008-05-31 18:23:01 UTC
In the updated patch, I added _WITH_CODE variants like for we have for G_DEFINE_TYPE.
Comment 21 Christian Persch 2008-06-22 17:23:45 UTC
Created attachment 113209 [details] [review]
[PATCH] Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]. Bug #449565.

 docs/reference/gobject/gobject-sections.txt |    4 +
 gobject/gtype.h                             |   82 +++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 0 deletions(-)
Comment 22 Christian Dywan 2008-09-08 13:00:37 UTC
This seems like a useful feature, the patch looks about right, can someone who has the expertise review the patch?
Comment 23 Behdad Esfahbod 2008-09-08 18:02:56 UTC
Tim?  This fixes real issues in almost every gobject-using code.

Lets do a QUARK one too.  The more places we can make thread-safe initializations automatic the better.
Comment 24 Dan Winship 2008-09-08 18:27:58 UTC
(In reply to comment #12)
> If the copy and free funcs are public (which they usually will be), they'll
> take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in
> the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient...

And G_DEFINE_TYPE does the casts for you on the init and class_init functions, so it's consistent to have G_DEFINE_BOXED_TYPE do it here too.
Comment 25 Matthias Clasen 2008-11-29 02:00:40 UTC
Tim, any final comment on this ? 
Comment 26 André Klapper 2009-01-02 20:38:41 UTC
*ping* Any news here? Tim?
Comment 27 André Klapper 2009-01-29 00:56:26 UTC
*ping* Any news here? Tim?
Comment 28 Behdad Esfahbod 2009-04-05 14:02:36 UTC
Can we also get a G_DEFINE_INTERFACE[_WITH_CODE] please?

And yet another ping.
Comment 29 André Klapper 2009-05-03 19:34:46 UTC
Ping!
Comment 30 Behdad Esfahbod 2009-07-28 16:43:19 UTC
Anyone want to prepare more patches, for INTERFACE, ENUM, FLAGS, and QUARK?
Comment 31 Dan Winship 2009-07-28 16:56:49 UTC
INTERFACE is bug 320482
Comment 32 Christian Persch 2009-12-01 17:16:23 UTC
Created attachment 148838 [details] [review]
updated patch to master
Comment 33 Christian Persch 2010-02-09 14:42:54 UTC
Ping? Any chance this could make it in for 2.24 ?
Comment 34 Tim Janik 2010-04-06 09:43:52 UTC
(In reply to comment #12)
> If the copy and free funcs are public (which they usually will be), they'll
> take a Foo* not a gpointer, so you'll _almost always_ have to put the cast in
> the G_DEFINE_BOXED_TYPE. That strikes me as rather inconvenient...

Good point.(In reply to comment #14)

> Since this is a convenience macro, wouldn't it be more convenient to implicitly
> form the copy/free function names (type_name##_copy, similar to
> type_name##_class_init) and in case their naming is different provide
> G_DEFINE_BOXED_TYPE_WITH_FUNCTIONS() where they can be explicitly given?

As has been pointed out, _copy and _ref are both in use for these functions, so canonical naming won't work here.

We can however still have the compiler check the function _type_, this works by casting the _register function so that it accepts function pointers of a particular boxed struct type. The technique to implement this is e.g. demonstrated in g_test_add(), which takes a type as argument (Fixture) and a function using this type (ftest).
Comment 35 Christian Persch 2010-04-06 11:57:45 UTC
You mean like this?

#define _G_DEFINE_BOXED_TYPE_BEGIN(TypeName, type_name, copy_func, free_func) \
GType \
type_name##_get_type (void) \
{ \
  static volatile gsize g_define_type_id__volatile = 0; \
  if (g_once_init_enter (&g_define_type_id__volatile))  \
    { \
      GType (* _g_register_boxed) (const gchar *, TypeName * (*) (TypeName *), void (*) (TypeName *)) = \
        (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*) (TypeName *))) g_boxed_type_register_static; \
      GType g_define_type_id = \
        _g_register_boxed (g_intern_static_string (#TypeName), copy_func, free_func); \
      { /* custom code follows */

I tried this and it works.

However, there are additional problems:

* constness. E.g. when using this to replace the hand-written register funcs in gboxed.c:

G_DEFINE_BOXED_TYPE (GValueArray, g_value_array, g_value_array_copy, g_value_array_free)
but:
GValueArray* g_value_array_copy(const GValueArray *value_array);

and 
G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, g_variant_type_free)
but:
GVariantType *g_variant_type_copy  (const GVariantType  *type);

so you get a compiler warning.


* TypeName* != C type name, e.g. for 

G_DEFINE_BOXED_TYPE (GStrv, g_strv, g_strdupv, g_strfreev)

where GStrv is gchar**, and g_strdupv/g_strfreev of course don't take GStrv*. This seems a rare problem, but would still be nice to handle it...


* renames. E.g. to get the existing type names for gvarianttype and gvariant's get_type functions, I had to do this:

#define g_variant_type_get_type g_variant_type_get_gtype
G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy, g_variant_type_free)
#undef g_variant_type_get_type
#define g_variant_get_type g_variant_get_gtype
G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref)
#undef g_variant_get_type

I guess that's the best we can do here, or should we make the name of the get_type function a param of the macro too?
Comment 36 Christian Persch 2010-04-06 12:02:19 UTC
Created attachment 158039 [details] [review]
updated patches
Comment 37 Dan Winship 2010-04-06 12:11:15 UTC
(In reply to comment #35)
> However, there are additional problems:
> 
> * constness. E.g. when using this to replace the hand-written register funcs in
> gboxed.c:

G_DEFINE_INTERFACE ended up making some assumptions that didn't work cleanly with gio's pre-existing interface type definitions, so there's precedent here.
Comment 38 Tim Janik 2010-04-06 12:18:05 UTC
(In reply to comment #35)
> You mean like this?
> 
> #define _G_DEFINE_BOXED_TYPE_BEGIN(TypeName, type_name, copy_func, free_func) \
> GType \
> type_name##_get_type (void) \
> { \
>   static volatile gsize g_define_type_id__volatile = 0; \
>   if (g_once_init_enter (&g_define_type_id__volatile))  \
>     { \
>       GType (* _g_register_boxed) (const gchar *, TypeName * (*) (TypeName *),

The variable declaration isn't really neccessary, function casts can be inlined, e.g.:
 ( (void(*)(void)) some_func ) ();

Your version is arguably more readable though.

> void (*) (TypeName *)) = \
>         (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*)

Using "TypeName* (*) (const TypeName*)" would really be the correct approach here and fix your const issues, it'd force const correctness on boxed type implementors though.

> * renames. E.g. to get the existing type names for gvarianttype and gvariant's
> get_type functions, I had to do this:
> 
> #define g_variant_type_get_type g_variant_type_get_gtype
> G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy,
> g_variant_type_free)
> #undef g_variant_type_get_type
> #define g_variant_get_type g_variant_get_gtype
> G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref)
> #undef g_variant_get_type
> 
> I guess that's the best we can do here, or should we make the name of the
> get_type function a param of the macro too?

I'd really like to avoid this, we also introduced similar macros for object types in order to *enforce* consistent function naming, could Ryan maybe comment here why the inconsistencies are needed?
Comment 39 Christian Persch 2010-04-06 12:28:08 UTC
(In reply to comment #38)
> > void (*) (TypeName *)) = \
> >         (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*)
> 
> Using "TypeName* (*) (const TypeName*)" would really be the correct approach
> here and fix your const issues, it'd force const correctness on boxed type
> implementors though.

But the copy_func isn't necessarily const, e.g. it will change the refcount for refcounted boxed types (GHashTable, GArray, GPtrArray,GClosure...).
Comment 40 Murray Cumming 2010-04-06 13:06:42 UTC
(In reply to comment #39)
> But the copy_func isn't necessarily const, e.g. it will change the refcount for
> refcounted boxed types (GHashTable, GArray, GPtrArray,GClosure...).

const can mean whatever the API developer decides it should mean. It shouldn't just mean that no bits will changed. For instance, an internal cache could change. gtkmm lets const objects be refed and unrefed (via its smartpointer) because anything else would make the const useless.

Well, that's the practice in C++, where const is fully used. But const just doesn't work in C, mostly because there are no const member functions and no mutable.
Comment 41 Christian Persch 2010-05-31 19:11:09 UTC
(In reply to comment #38)
> (In reply to comment #35)
> > void (*) (TypeName *)) = \
> >         (GType (*) (const gchar *, TypeName * (*) (TypeName *), void (*)
> 
> Using "TypeName* (*) (const TypeName*)" would really be the correct approach
> here and fix your const issues, it'd force const correctness on boxed type
> implementors though.

GBoxedCopyFunc uses gpointer not gconstpointer, so leaving the const off would be consistent. 

To proceed, should I:
* add |const|, and use explicit casts when using G_DEFINE_BOXED_TYPE with existing non-const copy funcs, or
* leave const off, and use explicit casts when using it with existing const copy funcs ?
(The former would introduce more casts than the latter in the uses of this macro in gobject/gio.)

> > * renames. E.g. to get the existing type names for gvarianttype and gvariant's
> > get_type functions, I had to do this:
> > 
> > #define g_variant_type_get_type g_variant_type_get_gtype
> > G_DEFINE_BOXED_TYPE (GVariantType, g_variant_type, g_variant_type_copy,
> > g_variant_type_free)
> > #undef g_variant_type_get_type
> > #define g_variant_get_type g_variant_get_gtype
> > G_DEFINE_BOXED_TYPE (GVariant, g_variant, g_variant_ref, g_variant_unref)
> > #undef g_variant_get_type
> > 
> > I guess that's the best we can do here, or should we make the name of the
> > get_type function a param of the macro too?
> 
> I'd really like to avoid this, we also introduced similar macros for object
> types in order to *enforce* consistent function naming, could Ryan maybe
> comment here why the inconsistencies are needed?

Whatever the reason was, this is now set in stone since it's part of stable 2.24.
Comment 42 Allison Karlitskaya (desrt) 2010-07-01 15:39:04 UTC
i suggest using transparent union types where available and discarding type safety elsewhere.

see bug 581023
Comment 43 Behdad Esfahbod 2010-07-01 15:56:06 UTC
Can we just get it in already?
Comment 44 Christian Persch 2010-07-01 18:22:08 UTC
Created attachment 165042 [details] [review]
Add G_DEFINE_{BOXED,POINTER}_TYPE[_WITH_CODE]

Add convenience type definition macros for boxed and pointer types
similar to G_DEFINE_TYPE for object types. Bug #449565.
Comment 45 Christian Persch 2010-07-01 18:24:01 UTC
Created attachment 165043 [details] [review]
Use G_DEFINE_[BOXED|POINTER]_TYPE instead of handwritten code

Now that we have convenience macros to implement boxed and pointer
types, use them.
Comment 46 Behdad Esfahbod 2010-07-01 18:26:27 UTC
Lets make sure these macros are "static"able, and document them so.
Comment 47 Christian Persch 2010-07-01 23:34:03 UTC
The latest patch in attachment 165042 [details] [review] fixes the const problem (comment 38 ff), by allowing to pass either a
 Foo * (*) (Foo *)
 Foo * (*) (const Foo *)
 GBoxedCopyFunc
as the copy func. Anything else will trigger a compiler warning. (Implemented using transparent unions, so only on gcc >= 2.7; on other compilers it will just cast to GBoxedCopyFunc).

Are this patch (and the follow-up to make use of it in glib) now ok to commit?
Comment 48 Christian Persch 2010-08-09 12:28:00 UTC
Ping, again?
Comment 49 Christian Persch 2010-08-17 22:15:09 UTC
Got ok-to-commit from #gtk-devel meeting. 

Pushed to master.
Comment 50 Christian Persch 2010-08-18 13:37:35 UTC
I filed the G_DEFINE_QUARK request (comment 4) as bug 627240, and the G_DEFINE_[ENUM|FLAGS]_TYPE request (comment 17) as bug 627241.
Comment 51 Kristian Rietveld 2010-09-13 17:22:16 UTC
While glib/GTK+ usually compile just fine with -pedantic (which Clang uses by default), this change seems to introduce warnings.

One of these warnings (there are many more of these of course), Clang emits:

stock_browser.c:62:1: warning: incompatible pointer types initializing 'GType
      (gchar const *, GBoxedCopyFunc, GBoxedFreeFunc)', expected 'GType
      (*)(gchar const *, union <anonymous>, union <anonymous>)' [-pedantic]
G_DEFINE_BOXED_TYPE (StockItemInfo, stock_item_info,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


gcc -pedantic gives:

stock_browser.c: In function 'stock_item_info_get_type':
stock_browser.c:62: warning: ISO C prohibits argument conversion to union type
stock_browser.c:62: warning: ISO C prohibits argument conversion to union type


Is this something that should and can be fixed?  Or is -pedantic too strict for glib/GTK+'s liking?
Comment 52 Christophe Fergeau 2011-03-22 10:17:10 UTC
Fwiw, this causes a warning when using clang default compile flags
Comment 53 Christophe Fergeau 2011-03-22 10:19:27 UTC
Created attachment 184042 [details]
Test case

When compiled using clang, this test case produces this warning:

./clang.c:30:1: warning: incompatible pointer types initializing 'GType
      (*)(gchar const *, union <anonymous at ./clang.c:30:1>, union <anonymous
      at ./clang.c:30:1>)' with an expression of type 'GType (gchar const *,
      GBoxedCopyFunc, GBoxedFreeFunc)'
G_DEFINE_BOXED_TYPE(BoxedString, boxed_string,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./clang.c:5:
In file included from /usr/include/glib-2.0/glib-object.h:25:
In file included from /usr/include/glib-2.0/gobject/gbinding.h:31:
In file included from /usr/include/glib-2.0/gobject/gobject.h:26:
/usr/include/glib-2.0/gobject/gtype.h:1521:72: note: instantiated from:
  ...copy_func, free_func) G_DEFINE_BOXED_TYPE_WITH_CODE (TypeName, ...
                           ^
/usr/include/glib-2.0/gobject/gtype.h:1538:87: note: instantiated from:
  ...free_func, _C_) _G_DEFINE_BOXED_TYPE_BEGIN (TypeName, type_name, ...
                     ^
/usr/include/glib-2.0/gobject/gtype.h:1548:16: note: instantiated from:
      GType (* _g_register_boxed) \
               ^
1 warning generated.

clang defines __GNU__ and __GNU_MINOR__ to 4 and 2 so it uses the #define with the union for G_DEFINE_BOXED_TYPE_BEGIN
Comment 54 Christian Persch 2011-03-22 13:27:38 UTC
I guess we could change the definition to use

#if (__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 7)) && !defined(__STRICT_ANSI__)

which works since if you pass -pedantic you most likely will also use -ansi (see gcc manual for -pedantic).
Comment 55 Christophe Fergeau 2011-03-22 13:42:15 UTC
This doesn't avoid the clang warning though.
Comment 56 Christian Persch 2011-03-22 13:55:45 UTC
Is there a clang-specific define we could check for to distinguish it from gcc ?
Comment 57 Christophe Fergeau 2011-03-22 14:24:04 UTC
According to google, clang -dM -E - < /dev/null dumps all preprocessor constants defined by clang. On my box, I have:

#define __clang__ 1
#define __clang_major__ 2
#define __clang_minor__ 8
#define __clang_patchlevel__ 0
#define __clang_version__ "2.8 (branches/release_28)"
#define __llvm__ 1

which could be useful here.