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 778219 - Glib::Variant: Suggested improvements to documentation of “maybe” types, etc.
Glib::Variant: Suggested improvements to documentation of “maybe” types, etc.
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: documentation
2.50.x
Other All
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2017-02-06 00:08 UTC by Daniel Boles
Modified: 2018-01-15 15:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH 1/3] Glib::Variant—Cosmetically tweak braces & newlines (3.46 KB, patch)
2017-02-06 00:08 UTC, Daniel Boles
committed Details | Review
[PATCH 2/3] Glib::Variant—Improve documentation of get_maybe() (1.29 KB, patch)
2017-02-06 00:09 UTC, Daniel Boles
committed Details | Review
[PATCH 3/3] Glib::Variant—Explain how to create “maybe” types (1.58 KB, patch)
2017-02-06 00:11 UTC, Daniel Boles
committed Details | Review
[PATCH 1/2] Variant: Add another usage hint for maybe Variants (1.09 KB, patch)
2017-02-15 08:29 UTC, Daniel Boles
committed Details | Review
[PATCH 2/2] Variant: Copy maybe docs to Variant< Variant<T> > (947 bytes, patch)
2017-02-15 08:30 UTC, Daniel Boles
committed Details | Review

Description Daniel Boles 2017-02-06 00:08: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.
Comment 1 Daniel Boles 2017-02-06 00:09:57 UTC
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.
Comment 2 Daniel Boles 2017-02-06 00:11:55 UTC
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!
Comment 3 Kjell Ahlstedt 2017-02-13 14:33:59 UTC
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.
Comment 4 Daniel Boles 2017-02-13 15:49:41 UTC
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.
Comment 5 Daniel Boles 2017-02-15 08:29:21 UTC
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.
Comment 6 Daniel Boles 2017-02-15 08:30:03 UTC
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.
Comment 7 Daniel Boles 2017-02-15 08:41:04 UTC
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...
Comment 8 Daniel Boles 2017-02-15 08:51:10 UTC
(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.
Comment 9 Kjell Ahlstedt 2017-02-15 15:54:22 UTC
Have you seen bug 694595? It's also about "maybe" types in Glib::Variant.
Comment 10 Murray Cumming 2017-02-23 11:29:56 UTC
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.