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 512717 - ValueBase classes "enhancements"
ValueBase classes "enhancements"
Status: RESOLVED OBSOLETE
Product: glibmm
Classification: Bindings
Component: general
2.15.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2008-01-29 05:19 UTC by José Alburquerque
Modified: 2018-05-22 12:08 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
ValueBase classes enhancements (13.29 KB, patch)
2008-01-29 05:20 UTC, José Alburquerque
none Details | Review
Enhancements with ChangeLog (14.01 KB, patch)
2008-01-29 06:58 UTC, José Alburquerque
none Details | Review
Patch with comments where necessary (14.65 KB, patch)
2008-02-06 00:52 UTC, José Alburquerque
none Details | Review

Description José Alburquerque 2008-01-29 05:19:06 UTC
Working on the GStreamermm Structure class, I noticed that it has setters and getters that set and return ValueBase objects for values which the underlying GstStructure (from GStreamer) contains.

When looking at the getters, I thought that it'd be nice if these getters simply returned a ValueBase instead of accepting a reference to the ValueBase (in which the value is returned) and then returning a boolean telling if the getters are successful in fetching the value.

To this end, I thought it'd really be nice if ValueBases could have a boolean like operator telling if it has been validly set.  I did some exploring and found an interesting link explaining how to implement safe boolean operators (http://www.artima.com/cppsource/safebool3.html) and adapted some of this explanation to implement a boolean operator for the ValueBase class.

I also thought that making the ValueBase classes "semi-automatic" so that they can be readily assigned to on declaration and in the course of a program without having to initialize them before hand would be kind of handy, so in the "set()" methods, I added two lines of code to make sure the Value class is initialized before being set.  I made all the ValueBase class descendants except for the Value_Enum and Value_Flags semi-automatic because I found that these last two cannot be automatically initialized.

The semi-automatic mechanism installed in the ValueBase classes does not affect code that already use the ValueBase class.  It is only triggered when setting ValueBase classes that have not been initialized.  If one is declared but never initialized or set, warnings are generated as usual and initialization before usage works just as it has before, so previous code never even notices the difference.

Lastly, I implemented cast and equal operators for easy syntactic usage in expressions.  The cast operators are very useful for using these Values as if they were, in fact, the actual type that they "wrap".  The pointer types also have dereferencing operators while the RefPtr types only have the member referencing (->) operator allowing similar RefPtr access to the underlying class of the RefPtr (just as a RefPtr would).

I did find ambiguity problems with the stream insertion operator (<<) and the Value classes.  This is because a boolean operator is included in the ValueBase class while the descendants have other cast operators that can be used in the same context.

Overall, however, I think that these changes would be handy to enhance the usage of the ValueBase classes so I was inclined to submit a patch.  However, I fully understand that there probably are an infinite number of things that I don't have perspective on, so submitting this patch I fully understand that it can be outright rejected because of something I haven't even considered.  All in all, if it is rejected, would be possible to keep the ValueBase operator?  I sincerely hope that some of this might be useful.  Thanks.
Comment 1 José Alburquerque 2008-01-29 05:20:17 UTC
Created attachment 103931 [details] [review]
ValueBase classes enhancements
Comment 2 José Alburquerque 2008-01-29 06:58:04 UTC
Created attachment 103936 [details] [review]
Enhancements with ChangeLog
Comment 3 Murray Cumming 2008-02-04 11:38:46 UTC
> The semi-automatic mechanism installed in the ValueBase classes does not affect
> code that already use the ValueBase class.  It is only triggered when setting
> ValueBase classes that have not been initialized.  If one is declared but 
> never initialized or set, warnings are generated as usual and initialization before
> usage works just as it has before, so previous code never even notices the
difference.

This is this code:

// Make sure class is initialized before setting
+  if (G_VALUE_TYPE(&gobject_) == 0)
+    init(value_type());

I don't think that would cause a warning, but that's OK - I wouldn't want it to. I would like this change to go in, separate from the other changes. But I suspect that the check should be in intit() instead of before using init().

I would like a  ValueBase::operator bool too, though it's not very useful now that it's unnecessary. However:
- I don't understand why you've used bool_type instead of bool there.
- I don't understand what's "safe" about it.
- I'd rather not have things like this without any explanatory comments:
+ typedef void (ValueBase::*bool_type)() const;
+  void void_function() const {}

Maybe you could submit a new patch for that.


Then let's look at what's left.
Comment 4 José Alburquerque 2008-02-06 00:49:37 UTC
(In reply to comment #3)

When I submitted this report, I was under the impression that the Gst::Structure getter methods should return a ValueBase and not a boolean.  After asking and considering I understand now that the getters should still return booleans and return the result (if any) in ValueBase reference parameters.  The boolean return allows for valid result testing so, as you say, the need for a boolean operator is not that needed.

Furthermore, the Gst::Structure::get_field() method is complemented by the has_field() method which also provides for field existance testing thus providing a way to check for fields also without needing a ValueBase bool operator.  So, again as you say, the operator may not be necessary;  However, I'll try to answer your observations below. 

> This is this code:
> 
> // Make sure class is initialized before setting
> +  if (G_VALUE_TYPE(&gobject_) == 0)
> +    init(value_type());
> 
> I don't think that would cause a warning, but that's OK - I wouldn't want it
> to. I would like this change to go in, separate from the other changes. But I
> suspect that the check should be in intit() instead of before using init().
> 

I'm questioning this patch's usefulness, but if it were to be used, the purpose of the code above is to allow setting (or assigning to) a Value without necessarily having to "init()" it first.  The code above (included in most "set()" methods) initializes the Value before setting it if the value has not been initialized already (G_VALUE_TYPE(...) == 0) (This is what I refer to as "semi-automatic" initialization).

The new patch I'm attaching removes warnings if a Value is declared, but never initialized or set by checking for this condition in the Value's destructor.

> I would like a  ValueBase::operator bool too, though it's not very useful now
> that it's unnecessary. However:
> - I don't understand why you've used bool_type instead of bool there.
> - I don't understand what's "safe" about it.

Initially, I tried using the regular operator bool(), but this creates arithmetic ambiguities with the stream insertion operator (<<) (making it difficult for casting operators to be recognized) so I thought of using the pointer operator (operator*) which can be used in boolean contexts but doesn't create arithmetic ambiguities with the insertion operator. Passing a pointer to the Value is what's not safe, however, because the value can be deleted this way and this is how the bool_type weirdness came about.  I found a "dissertation" on this and used the suggestions at http://www.artima.com/cppsource/safebool.html.

> - I'd rather not have things like this without any explanatory comments:
> + typedef void (ValueBase::*bool_type)() const;
> +  void void_function() const {}
> 
> Maybe you could submit a new patch for that.

I will comment in the new patch.

> 
> 
> Then let's look at what's left.
> 

I'm submitting the patch to follow through (it may or may not be useful and I'd rather let you or someone who has a bit more experience than I do determine that).  Please take your time deciding how useful (or not) this patch might be (or not) and feel free to modify it.  Be it as it may, I'm perfectly happy because as I already said that you've said, a bool operator may not be that necessary.  Thanks.
Comment 5 José Alburquerque 2008-02-06 00:52:00 UTC
Created attachment 104530 [details] [review]
Patch with comments where necessary
Comment 6 Murray Cumming 2008-02-20 13:13:09 UTC
It's good to have this in bugzilla, but I don't think I'll apply it until someone needs it and can therefore be a good tester of it.
Comment 7 Daniel Boles 2017-08-13 15:53:33 UTC
(In reply to José Alburquerque from comment #0)
> When looking at the getters, I thought that it'd be nice if these getters
> simply returned a ValueBase instead of accepting a reference to the
> ValueBase (in which the value is returned) and then returning a boolean
> telling if the getters are successful in fetching the value.

Is the reference argument ever used polymorphically? If so, returning a ValueBase by value would lead to object slicing and presumably break something. In situations where I've seen get(object&), it's been for e.g. Glib::Variant, where you'd really pass the function a derived class, but it sets it using base methods.


> To this end, I thought it'd really be nice if ValueBases could have a
> boolean like operator telling if it has been validly set.

I assume that, by now, glibmm-2.4 will have to stay how it is.

glibmm-next, however, could look at using C++17 features, to alleviate further the need for a bool operator, for example:
 * return an std::optional<ValueBase>
 * return an std::pair<bool, ValueBase>, which will not be as terrible
   to work with as it is in C++14, since C++17 adds structured bindings:
     if (auto [valid, value] = thing.get(); valid) { doStuffWith(value); }
Comment 8 GNOME Infrastructure Team 2018-05-22 12:08:03 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glibmm/issues/5.