[Bug 30423] Provide convenience API for GVariant-based a{sv}s

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30423

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-10-12 03:58:02 PDT ---
(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);
    }

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.