Bug 30423 (TpVariantMap)

Review thoughts:

+ * If @dict is a floating reference, ownership will be taken as if via
+ * g_variant_ref_sink(). Otherwise, ownership will not be taken.

This is very confusingly worded. As far as I know, it always holds a reference, but seems to imply it will not.

+TpVariantMap *
+tp_variant_map_new_frozen (GVariant *dict)

You don't think new_from_variant() is a clearer name? You can explain in the text that it's frozen. Or new_frozen_from_variant(), though that's getting long.

Why are TpVariantMaps not reference counted? Like hash tables, they seem like the sort of thing you may wish to reference count.

There's set() and set_value(), and get_value(), but no get().

+TpVariantMap *
+tp_variant_map_copy (TpVariantMap *self)

This is the only way to unfreeze a variant map, which is potentially a pain in the neck. It seems like there's also use for an unfreeze() method that works similarly to this method... but dropping the ref on the variant. If that exists, then perhaps copy() should just make an exact copy, which would be less confusing.

I wonder about 86b2b165bfde719fa9c5401b4dda15a530274740 .. I see what you're doing, but I'm worried about people doing: set_value (set_value (set_value (set_value (tp_variant_new (), v1), v2), ... ) -- everyone becomes a closet Lisp programmer. I was going to suggest making tp_variant_map_new() behave like tp_asv_new(), but I don't like that idea either.

(In reply to comment #1)
> Review thoughts:
> 
> + * If @dict is a floating reference, ownership will be taken as if via
> + * g_variant_ref_sink(). Otherwise, ownership will not be taken.
> 
> This is very confusingly worded. As far as I know, it always holds a reference,
> but seems to imply it will not.

If @dict is floating (see the GVariant docs for what that means), the caller no longer owns it after the call; if the caller previously had a "real reference", the caller gets to keep it. It's the same as g_variant_new_variant() and g_variant_ref_sink(). Any suggestions for better wording?

g_variant_new_variant says "If @child is a floating reference (see g_variant_ref_sink()), the new instance takes ownership of @child." Would that (with s/child/dict/) be clearer?

> +TpVariantMap *
> +tp_variant_map_new_frozen (GVariant *dict)
> 
> You don't think new_from_variant() is a clearer name? You can explain in the
> text that it's frozen. Or new_frozen_from_variant(), though that's getting
> long.

I'm not sure that new_from_variant would be clearer, no; the important difference is that this one can't be changed.

> Why are TpVariantMaps not reference counted? Like hash tables, they seem like
> the sort of thing you may wish to reference count.

That seems reasonable.

However, should the GBoxedCopyFunc be ref, deep-copy, or a hybrid function that deep-copies non-frozen maps but just refs frozen ones?

If it's ref, then boxed-copying a non-frozen TpVariantMap (e.g. through a GValue) gives you "action at a distance" - the receiver can alter the sender's TpVariantMap, which seems undesirable for a value-object - and doesn't do the same thing as the copy() method.

If it's deep-copy, it's a bit of CPU/memory churn, but note that the keys (strings) are copied, but the values (arbitrary variants) are just reffed.

A hybrid function could be extremely efficient but I suspect it's a bit surprising: I'd tend to expect that copying an unsafe-to-modify thing is the way you get your own modifiable thing.

> There's set() and set_value(), and get_value(), but no get().

set() is safe, because having the wrong varargs would be programming error.

get() with varargs and format strings is either unsafe, because you're failing to check the types, or heavily limited, because we can only do a type-checked get() for types we specifically understand (in which case you probably want to do some limited type coercion, like get_uint32 does, to be nice to deficient bindings like dbus-glib).

> +TpVariantMap *
> +tp_variant_map_copy (TpVariantMap *self)
> 
> This is the only way to unfreeze a variant map, which is potentially a pain in
> the neck. It seems like there's also use for an unfreeze() method that works
> similarly to this method... but dropping the ref on the variant. If that
> exists, then perhaps copy() should just make an exact copy, which would be less
> confusing.

I deliberately didn't add an unfreeze method: once frozen, a TpVariantMap is just as immutable as the underlying GVariant. The general idea was that things like tp_channel_borrow_immutable_properties() would return a frozen map; if you want to modify it, you make your own copy and do things with that instead.

Perhaps the frozen and thawed maps should in principle be different types, but I'd rather not have to have two copies of all the getters...

See also discussion with desrt on GNOME bug 625408, linked above. I implemented this myself because it seemed likely to be quicker than waiting for him.

> I wonder about 86b2b165bfde719fa9c5401b4dda15a530274740 .. I see what you're
> doing, but I'm worried about people doing: set_value (set_value (set_value
> (set_value (tp_variant_new (), v1), v2), ... ) -- everyone becomes a closet
> Lisp programmer.

My thought was that it doesn't cost anything, and could help clarity in less pathological cases. If people want to set n values in C they should use the varargs version anyway (tp_variant_map_set()).

In g-i (or even C++) it'd come out more like:

    return (VariantMap.new()
        .set_value("badger", Variant.new_int(42))
        .set_value("mushroom", Variant.new_int(23))
        .set_value("snake", Variant.new_int(5)))

which I think is about as good as you're going to get without language-specific syntactic sugar (i.e. literals).

> I was going to suggest making tp_variant_map_new() behave like
> tp_asv_new(), but I don't like that idea either.

No, I think people who want an equivalent of tp_asv_new() should just use tp_variant_map_new() followed by a many-argument call to tp_variant_map_set().

Because of the chaining, you can even combine them if you feel like it:

    TpVariantMap *
    get_some_rubbish_or_other (void)
    {
      return tp_variant_map_set (tp_variant_map_new (),
          "string", "s", "o hai",
          "number", "i", 42,
          NULL);
    }

(In reply to comment #2)
> (In reply to comment #1)
> > Review thoughts:
> > 
> > + * If @dict is a floating reference, ownership will be taken as if via
> > + * g_variant_ref_sink(). Otherwise, ownership will not be taken.
> > 
> > This is very confusingly worded. As far as I know, it always holds a reference,
> > but seems to imply it will not.
> 
> If @dict is floating (see the GVariant docs for what that means), the caller no
> longer owns it after the call; if the caller previously had a "real reference",
> the caller gets to keep it. It's the same as g_variant_new_variant() and
> g_variant_ref_sink(). Any suggestions for better wording?

> g_variant_new_variant says "If @child is a floating reference (see
> g_variant_ref_sink()), the new instance takes ownership of @child." Would that
> (with s/child/dict/) be clearer?

Perhaps: A reference to @dict is taken via g_variant_ref_sink(). If @dict is a floating reference, the new #TpVariantMap takes ownership of @dict.

> > There's set() and set_value(), and get_value(), but no get().
> 
> set() is safe, because having the wrong varargs would be programming error.
> 
> get() with varargs and format strings is either unsafe, because you're failing
> to check the types, or heavily limited, because we can only do a type-checked
> get() for types we specifically understand (in which case you probably want to
> do some limited type coercion, like get_uint32 does, to be nice to deficient
> bindings like dbus-glib).

Reasonable.

> > +TpVariantMap *
> > +tp_variant_map_copy (TpVariantMap *self)
> > 
> > This is the only way to unfreeze a variant map, which is potentially a pain in
> > the neck. It seems like there's also use for an unfreeze() method that works
> > similarly to this method... but dropping the ref on the variant. If that
> > exists, then perhaps copy() should just make an exact copy, which would be less
> > confusing.
> 
> I deliberately didn't add an unfreeze method: once frozen, a TpVariantMap is
> just as immutable as the underlying GVariant. The general idea was that things
> like tp_channel_borrow_immutable_properties() would return a frozen map; if you
> want to modify it, you make your own copy and do things with that instead.
> 
> Perhaps the frozen and thawed maps should in principle be different types, but
> I'd rather not have to have two copies of all the getters...
> 
> See also discussion with desrt on GNOME bug 625408, linked above. I implemented
> this myself because it seemed likely to be quicker than waiting for him.

Ok, that's reasonable. What about cases where you want to pass a TpVariantMap to a function, but then continue to make changes to it (e.g. launching several channels with the same TargetID), which is something that's possible with a{sv} maps at the moment.

You might have something like:

map = tp_variant_map_new ();
map.set (TP_PROP_CHANNEL_TARGET_HANDLE, "u", TP_HANDLE_TYPE_ROOM,
         TP_PROP_CHANNEL_TARGET_ID, "s", "kittens@conf.litter.cat",
         NULL);

map.set (TP_PROP_CHANNEL_CHANNEL_TYPE, "s", TP_IFACE_CHANNEL_TYPE_TEXT, NULL);
ensure_channel (map);

map.set (TP_PROP_CHANNEL_CHANNEL_TYPE, "s", TP_IFACE_CHANNEL_TYPE_DBUS_TUBE,
         TP_PROP_CHANNEL_TYPE_DBUS_TUBE_SERVICE_NAME, "s", "cat.litter.Catlaboration",
         NULL);
create_channel (map);

Is any function that accepts a TpVariantMap going to freeze your map on you? Perhaps immutability needs to be separated from building the variant?

(In reply to comment #3)
> Perhaps: A reference to @dict is taken via g_variant_ref_sink(). If @dict is a
> floating reference, the new #TpVariantMap takes ownership of @dict.

That sounds good wording, thanks.

> What about cases where you want to pass a TpVariantMap
> to a function, but then continue to make changes to it (e.g. launching several
> channels with the same TargetID), which is something that's possible with a{sv}
> maps at the moment.

That's an interesting case.

> Is any function that accepts a TpVariantMap going to freeze your map on you?
> Perhaps immutability needs to be separated from building the variant?

Potentially, yes: when you send a request off onto D-Bus, it'll need to be serialized into a GVariant. Hmm.

Perhaps instead we ought to have a vaguely Qt-like copy-on-write mechanism in which instances look as though they're all independent, but in fact they share data behind the scenes as much as possible. Something like this:

- TpVariantMap is a boxed, mutable value-type, and is not refcounted

- a TpVariantMap contains a hash table, a GVariant or both

- "copying" a TpVariantMap ensures that its GVariant and hash table exist, then puts a ref to each in the copy

- whenever a TpVariantMap is modified, if the GVariant exists, it first makes itself independent of any other copies, which means deep-copying its own hash table (strdup keys, ref values) (or setting up a new hash table if it doesn't have one), and setting the GVariant to NULL

(Or, we could have the "I am independent" flag be explicit, rather than using presence/absence of the GVariant.)

Then we could get rid of the concept of freezing altogether, and just have:

GVariant *
tp_variant_map_dup_variant (TpVariantMap *self)
{
  if (self->variant == NULL)
    {
      /* reconstitute the GVariant from the GHashTable */
    }

  return g_variant_ref (self->variant);
}

There could even be a version that borrows a GVariant valid until the TpVariantMap is next modified, although that might be a bit error-prone.

Adding Guillaume to Cc since this is hopefully useful for TpMessage.

(In reply to comment #4)
> Perhaps instead we ought to have a vaguely Qt-like copy-on-write mechanism in
> which instances look as though they're all independent, but in fact they share
> data behind the scenes as much as possible.

My variant-map-cow branch switches to these semantics. The cost of tp_variant_map_copy() is now O(1):

* slice-allocate a 3-word struct
* ref a variant
* ref a hash table

so I don't think it needs separate refcounting.

> (Or, we could have the "I am independent" flag be explicit, rather than using
> presence/absence of the GVariant.)

I think this is probably better, and have implemented it.

> GVariant *
> tp_variant_map_dup_variant (TpVariantMap *self)

I called it tp_variant_map_serialize(), but same difference.

This should be in GLib. desrt provided g_variant_lookup(), which isn't exactly what I wanted, but it's close enough.