GNOME Bugzilla – Bug 778219
Glib::Variant: Suggested improvements to documentation of “maybe” types, etc.
Last modified: 2018-01-15 15:39:38 UTC
Created attachment 345000 [details] [review] [PATCH 1/3] Glib::Variant—Cosmetically tweak braces & newlines [PATCH 1/3] > Donʼt use braces for single-line blocks, and do if the other side of an > if/else already did. Also, get rid of a couple of extraneous newlines.
Created attachment 345001 [details] [review] [PATCH 2/3] Glib::Variant—Improve documentation of get_maybe() > Clarify what is set and returned, and use lower case “nothing” to match > how that must be written in GVariant text format, à la GSettings, etc.
Created attachment 345002 [details] [review] [PATCH 3/3] Glib::Variant—Explain how to create “maybe” types Hopefully I'm not the only person who would ever get confused by this... > It’s not exactly intuitive why the *_maybe() methods are kept in > ContainerBase, nor that Variant<VariantBase> inherits from the former > and is the way to get a maybe-typed Variant in glibmm. Let’s fix that!
All three patches look fine. I just find the first (cosmetic) one somewhat unnecessary, but if you think it's worth doing, it's fine for me. Just one comment: When a patch has been discussed in a bug, it's fine to have the bug number in the commit message. Not that important in these simple patches, but generally it's nice to be able to easily find more information about a commit, if there is any. These patches won't change ABI or API. You can push them to both the master branch and the glibmm-2-50 branch, if you like.
Thanks as always for the reviews! Pushed to both. Also, good point about the URL: I normally keep the first post patch-free specifically so that I can get the URL before doing git format-patch, but I didn't do that this time for some reason. I fixed that before pushing.
Created attachment 345779 [details] [review] [PATCH 1/2] Variant: Add another usage hint for maybe Variants get_n_children() can be used to check if the current value is nothing.
Created attachment 345780 [details] [review] [PATCH 2/2] Variant: Copy maybe docs to Variant< Variant<T> > The paragraph from Variant<VariantBase> is equally applicable here.
Hm, maybe not. I expected that an alternative way to check/get the child of a maybe-typed Variant, e.g. Variant< Variant<int> > was * do get_n_children() * and if the result is 1, do Variant< Variant<T> >::get() i.e. I expected that the more specific Variant< Variant<T> > would let us get() the Variant<T> from it, after checking it's not nothing. But I get a bunch of type warnings doing that. So until I'm confident that I've figured out what Variant< Variant<T> > is useful for, and whether that includes maybe types, I'd leave these commits out. Currently, it looks like Variant<VariantBase> already has the useful stuff for dealing with maybe types (once you figure out how that's done) and Variant< Variant<T> > does not offer anything more there. But I'm probably wrong...
(In reply to Daniel Boles from comment #7) > Hm, maybe not. I expected that an alternative way to check/get the child of > a maybe-typed Variant, e.g. Variant< Variant<int> > was > * do get_n_children() > * and if the result is 1, do Variant< Variant<T> >::get() > > i.e. I expected that the more specific Variant< Variant<T> > would let us > get() the Variant<T> from it, after checking it's not nothing. > > But I get a bunch of type warnings doing that. You can instead follow get_n_children() by get_child(), if the former returned 1, since get_child() unlike get() is not type-checked. But this returns a VariantBase, so it's no closer to the goal of getting a Variant<int> or whatever without having to cast ourselves. Doing get_maybe(VariantBase&) is actually preferable here in that it achieves the same thing in less code, while also letting the target really be a more derived type. What I do is to declare a Variant<int>, try get_maybe() with that passed in, and if it returns true, directly do get() on the Variant<int> to get the int.
Have you seen bug 694595? It's also about "maybe" types in Glib::Variant.
Review of attachment 345000 [details] [review]: If we really want to change this then we should make the change in our clang-format file: https://git.gnome.org/browse/glibmm/tree/.clang-format Otherwise, I'd prefer to avoid these minor changes. Thanks.