I don't like that after you assign to _INIT, you must use _init().

I think we could make a better version of this that takes the type as an argument and writes it into the structure in a place that causes the builder to be initialised on first use.

This might be a bit tricky because we will need to have a constant as the type, which means that we cannot use G_VARIANT_TYPE() here.  Perhaps we should allow the possibility of giving a type string directly.

In any case, it would be fast, since it only involves putting a pointer to a string into a struct.

Created attachment 327790 [details] [review]
Adds the G_VARIANT_BUILDER_CLEARED macro.

For now I'm attaching a revised patch that renames the _INIT macro to _CLEARED, so we don't have _init after _INIT.

Created attachment 327803 [details] [review]
Adds the G_VARIANT_BUILDER_CLEARED macro.

Created attachment 327804 [details] [review]
Adds the G_VARIANT_DICT_CLEARED macro.

Created attachment 327805 [details] [review]
Adds the G_VARIANT_BUILDER_INIT macro.

I just attached three patches, one adds the G_VARIANT_BUILDER_CLEARED macro, one adds the same, but for GVariantDicts and the last adds the G_VARIANT_BUILDER_INIT(variant_type_string).

The last one is a bit rough on the edges and I have still to add some tests. But initial reviews are very welcome.

Review of attachment 327805 [details] [review]:

Great job.  This is pretty much exactly what I had in mind, but I think the changes introduced to the .c file are a bit too drastic.  In particular, I'd take this approach instead:

1) keep the current is_valid_builder() macro

2) add a new function called ensure_valid_builder() which calls is_valid_builder() first, returning TRUE if it is already valid.  if it's not true, then check for x[0] == PARTIAL_MAGIC and read x[1] as a 'const gchar *' via GSIZE_TO_POINTER.  Read from the public struct type here, since this is the one that was filled in.  Don't rely on the congruences between it and the members of the private struct (which someone may move around some day).  Use G_VARIANT_TYPE() to cast that string (with checks) to a GVariantType and just pass that directly to the normal g_variant_builder_init() call.  No need to add a new internal helper for this.  Finally, return is_valid_builder() [which will return false if either G_VARIANT_TYPE() or g_variant_builder_init() had failed for some reason).

3) replace all is_valid_builder() calls with ensure_valid_builder() calls.


We could also discuss another alternative if you have an improved counterproposal.

::: glib/gvariant.h
@@ +35,3 @@
+
+/* magic parts, do not use */
+#define GVSB_MAGIC_PARTIAL ((gsize) 2942751021u)

I don't like this as a public define, even though the alternative is duplication (which I usually like even less).  Please handle it this way instead:

 1) hardcode the value into the _INIT macro

 2) move this define for GVSB_MAGIC_PARTIAL to the .c file

 3) make sure these are kept in sync by way of a testcase which uses G_VARIANT_BUILDER_INIT

@@ +371,3 @@
+ *
+ * |[
+ *   g_auto(GVariantBuilder) builder = G_VARIANT_BUILDER_INIT("ay");

I think it might also be nice to allow type constants here, like G_VARIANT_TYPE_VARDICT for example.  I'd also prefer more type safety, so that if you pass things that are neither 'const gchar *' nor 'const GVariantType *' then you will get a warning about it.  Those two goals are at odds with each other, but there is probably some way to get what we want, possibly involving a transparent union...

Review of attachment 327803 [details] [review]:

I think this is not very important if we end up with the _INIT macro.

(In reply to Allison Ryan Lortie (desrt) from comment #6)
> Review of attachment 327805 [details] [review] [review]:
> 
> Great job.  This is pretty much exactly what I had in mind, but I think the
> changes introduced to the .c file are a bit too drastic.  In particular, I'd
> take this approach instead:
> 
> 1) keep the current is_valid_builder() macro
> 
> 2) add a new function called ensure_valid_builder() which calls
> is_valid_builder() first, returning TRUE if it is already valid.  if it's
> not true, then check for x[0] == PARTIAL_MAGIC and read x[1] as a 'const
> gchar *' via GSIZE_TO_POINTER.  Read from the public struct type here, since
> this is the one that was filled in.  Don't rely on the congruences between
> it and the members of the private struct (which someone may move around some
> day).  Use G_VARIANT_TYPE() to cast that string (with checks) to a
> GVariantType and just pass that directly to the normal
> g_variant_builder_init() call.  No need to add a new internal helper for
> this.  Finally, return is_valid_builder() [which will return false if either
> G_VARIANT_TYPE() or g_variant_builder_init() had failed for some reason).
> 

I would still like to ensure that x[2-15] are zeros to avoid a case, when we call g_variant_builder_open (...) and parent is set to address that is equal to our magic number. This is why I was making sure that GVSB(builder)->magic == 0.

> 3) replace all is_valid_builder() calls with ensure_valid_builder() calls.
> 
> 
> We could also discuss another alternative if you have an improved
> counterproposal.
> 

The plan looks good, it is not very different from what I did in the first place. This scenario has fewer changes even (SLOC-wise).

> ::: glib/gvariant.h
> @@ +35,3 @@
> +
> +/* magic parts, do not use */
> +#define GVSB_MAGIC_PARTIAL ((gsize) 2942751021u)
> 
> I don't like this as a public define, even though the alternative is
> duplication (which I usually like even less).  Please handle it this way
> instead:
> 
>  1) hardcode the value into the _INIT macro
> 
>  2) move this define for GVSB_MAGIC_PARTIAL to the .c file
> 
>  3) make sure these are kept in sync by way of a testcase which uses
> G_VARIANT_BUILDER_INIT
> 

Sure, added.

> @@ +371,3 @@
> + *
> + * |[
> + *   g_auto(GVariantBuilder) builder = G_VARIANT_BUILDER_INIT("ay");
> 
> I think it might also be nice to allow type constants here, like
> G_VARIANT_TYPE_VARDICT for example.  I'd also prefer more type safety, so
> that if you pass things that are neither 'const gchar *' nor 'const
> GVariantType *' then you will get a warning about it.  Those two goals are
> at odds with each other, but there is probably some way to get what we want,
> possibly involving a transparent union...

I was able to make it a bit more type-safe by adding a union. But I have no idea how to make the macro to accept only const gchar * and const GVariantType * without warnings.

Created attachment 327826 [details] [review]
Adds the G_VARIANT_BUILDER_INIT macro.

Created attachment 327827 [details] [review]
Adds the G_VARIANT_DICT_INIT macro.

Attached also a patch with the G_VARIANT_DICT_INIT macro.

Ah, and about _CLEARED macros - I dunno, they might be usable in scenarios like:

g_auto(GVariantBuilder) builder = G_VARIANT_BUILDER_CLEARED;

if (do_dict)
  g_variant_builder_init (&builder, G_VARIANT_TYPE("a{sv}"));
else if (do_pseudo_dict)
  g_variant_builder_init (&builder, G_VARIANT_TYPE("a(sv)"));
else
  return;

But arguably this is pretty strained reason. So, if you want, I'll drop the patches introducing the _CLEARED macros and drop the use of them in the patches that add the _INIT macros.

Also, one catch with _INIT macros is the lifetime issue of the parameter passed to the macro:

GVariantType *type = get_copy_of_some_variant_type ();
GVariantBuilder builder = G_VARIANT_BUILDER_INIT (type);

g_variant_type_free (type);
/* GVariantBuilder will try to copy the freed variant type */
g_variant_builder_add_value (&builder, ...);

As far as I'm concerned, this macro is only intended for use with type strings or G_VARIANT_TYPE_VARDICT-style constants (either defined by GLib, or by the user in the same way).  ie: static strings.  We should make the docs very clear on that point.

I also consider the case you list above (pseudo_dict) to be pretty borderline -- particularly considering it would mean that you have to open/close the builder many times... or make use of the pseudo_dict flag elsewhere.  If someone really wants to do that then they can avoid the g_auto() or just clear it manually, or make sure they avoid the early-return.

I'm still holding out hope that one day we will get an attribute in GCC that says "initialise this local variable to all-zeros unless otherwise specified" that we will be able to use for g_auto()....

(In reply to Allison Ryan Lortie (desrt) from comment #12)
> As far as I'm concerned, this macro is only intended for use with type
> strings or G_VARIANT_TYPE_VARDICT-style constants (either defined by GLib,
> or by the user in the same way).  ie: static strings.  We should make the
> docs very clear on that point.

Sure. What about G_VARIANT_DICT_INIT? It has similar issue with its GVariant* parameter, but without similar solution (other than documenting, that is).

> 
> I also consider the case you list above (pseudo_dict) to be pretty
> borderline -- particularly considering it would mean that you have to
> open/close the builder many times... or make use of the pseudo_dict flag
> elsewhere.  If someone really wants to do that then they can avoid the
> g_auto() or just clear it manually, or make sure they avoid the early-return.
> 

Yeah. Alright, I'll remove the _CLEARED macros.

> I'm still holding out hope that one day we will get an attribute in GCC that
> says "initialise this local variable to all-zeros unless otherwise
> specified" that we will be able to use for g_auto()....

Yup, would be cool to have that.

Created attachment 329119 [details] [review]
Adds the G_VARIANT_BUILDER_INIT macro.

Created attachment 329120 [details] [review]
Adds the G_VARIANT_DICT_INIT macro.

Updated the patches. I dropped the _CLEARED macros. Updated the docs for both G_VARIANT_BUILDER_INIT and G_VARIANT_DICT_INIT, but the docs for the latter are a bit mouthy, so suggestions for improvements are very welcome.

When I try to build glibmm, I now get the errors

/opt/gnome/include/glib-2.0/glib/gvariant.h:306:5: error: ISO C++ prohibits anonymous structs [-Werror=pedantic]
     };
     ^
/opt/gnome/include/glib-2.0/glib/gvariant.h:445:5: error: ISO C++ prohibits anonymous structs [-Werror=pedantic]
     };
     ^

It's easy to fix. Like this, for instance:

    struct {
      gsize partial_magic;
      const GVariantType *type;
      gsize y[14];
    } s;              // <-------

    struct {
      GVariant *asv;
      gsize partial_magic;
      gsize y[14];
    } s;              // <------

I see these warnings too, which breaks the glibmm --enable-warnings=fatal build. It would be great if we could avoid that. Is there any downside to giving these structs names?

However just giving them names introduces another error:

/opt/gnome/include/glib-2.0/glib/gvariant.h:302:12: error: ‘struct _GVariantBuilder::<anonymous union>::s1’ invalid; an anonymous union can only have non-static data members [-fpermissive]
     struct s1 {

Created attachment 331867 [details] [review]
Fixx C++ issues with gvariant.h

Murray,

Does this patch help? Quickly testing the following code resulted in no warnings/errors with g++ -c -Wall -Wextra -Wpedantic `pkg-config --cflags glib-2.0` test.cc with gcc 6.1.1

#include <glib.h>

int main()
{
  GVariantBuilder builder = G_VARIANT_BUILDER_INIT(G_VARIANT_TYPE_BYTESTRING);
  (void)builder;
}

Of course, yes, that seems fine. When using that patch on glibmm, I don't see the warning anymore when building glibmm. Thanks.

I mean, when using that patch on glib.

Applied the patch.

FWIW, downstream workaround to have code build both on <2.50 and >= 2.50:

https://github.com/ostreedev/ostree/pull/547