Menu ▾ ▴

#5349 Constness of Grob methods

Shelved
Dan Eble
None
Maintainability
2018-06-20
2018-06-18
Dan Eble
No

Grob::internal_get_property is advertised as a const method, but it came up in a code review that "caching of values is not really conceptually const." So, at the very least, the const is misleading. It would be good to change it for clarity, but it might require a chain of consequential changes.

More:

get_property_data is logically const (and used in a number of places to avoid premature callback evaluation). Stream event and music properties, I think, also are. get_object is, I think. get_pure_property may be (not entirely sure about that one, though).

We've had a number of early evaluation bugs, too, it's not really an academical distinction. I'm not against making "const" more useful for deciding things but exactly in that case we want Grob::get_property_internal not be const.

Discussion

  • Anonymous

    Anonymous - 2018-06-19
    • Description has changed:

    Diff:
    • Needs: -->
    • Patch: new --> review
     

  • Anonymous

    Anonymous - 2018-06-19

    Passes make, make check and a full make doc.

     

  • Dan Eble

    Dan Eble - 2018-06-20
    • summary: Use more const Grobs in the Paper_column interface --> Constness of Grob methods
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -1,3 +1 @@
-Use more const Grobs in the Paper_column interface
-
-http://codereview.appspot.com/346100043
+Grob::internal_get_property is advertised as a const method, but it came up in a [code review](http://codereview.appspot.com/346100043) that "caching of values is not really conceptually const."  So, at the very least, the const is misleading.  It would be good to change it for clarity, but it might require a chain of consequential changes.
    • status: Started --> Shelved
    • Patch: review -->
    • Type: Enhancement --> Maintainability
     

  • Dan Eble

    Dan Eble - 2018-06-20
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -1 +1,6 @@
 Grob::internal_get_property is advertised as a const method, but it came up in a [code review](http://codereview.appspot.com/346100043) that "caching of values is not really conceptually const."  So, at the very least, the const is misleading.  It would be good to change it for clarity, but it might require a chain of consequential changes.
+
+More:
+> get_property_data is logically const (and used in a number of places to avoid premature callback evaluation).  Stream event and music properties, I think, also are.  get_object is, I think. get_pure_property may be (not entirely sure about that one, though).
+> 
+> We've had a number of early evaluation bugs, too, it's not really an academical distinction.  I'm not against making "const" more useful for deciding things but exactly in that case we want Grob::get_property_internal _not_ be const.