Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2972)

Issue 346100043: Use more const Grobs in the Paper_column interface (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by Dan Eble
Modified:
5 years, 9 months ago
Reviewers:
dak
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

https://sourceforge.net/p/testlilyissues/issues/5349/ Nothing specific triggered this; I just noticed that the names of these functions sound like they should not mutate the Grobs.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -12 lines) Patch
M lily/include/paper-column.hh View 1 chunk +8 lines, -6 lines 0 comments Download
M lily/paper-column.cc View 6 chunks +6 lines, -6 lines 3 comments Download

Messages

Total messages: 5
dak
https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc File lily/paper-column.cc (right): https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc#newcode105 lily/paper-column.cc:105: SCM m = me->get_property ("when"); How much sense does ...
5 years, 9 months ago (2018-06-19 06:08:56 UTC) #1
Dan Eble
https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc File lily/paper-column.cc (right): https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc#newcode105 lily/paper-column.cc:105: SCM m = me->get_property ("when"); On 2018/06/19 06:08:56, dak ...
5 years, 9 months ago (2018-06-19 22:41:10 UTC) #2
dak
https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc File lily/paper-column.cc (right): https://codereview.appspot.com/346100043/diff/1/lily/paper-column.cc#newcode105 lily/paper-column.cc:105: SCM m = me->get_property ("when"); On 2018/06/19 22:41:09, Dan ...
5 years, 9 months ago (2018-06-19 23:15:42 UTC) #3
Dan Eble
On 2018/06/19 23:15:42, dak wrote: > evaluation. Due to the significant timing constraints caching of ...
5 years, 9 months ago (2018-06-19 23:44:57 UTC) #4
dak
5 years, 9 months ago (2018-06-20 06:48:25 UTC) #5
Message was sent while issue was closed.
On 2018/06/19 23:44:57, Dan Eble wrote:
> On 2018/06/19 23:15:42, dak wrote:
> > evaluation.  Due to the significant timing constraints caching of values is
> not
> > really conceptually const: in fact, a number of properties are evaluated
> solely
> 
> OK, well if get_property() is not even logically const (and if it isn't, what
> is?) then the thing to do is to remove const from it, but I'm not going to go
> there now.  I don't want to clean up after that.
> 
> I'll withdraw this patch.

get_property_data is logically const (and used in a number of places to avoid
premature callback evaluatino).  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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b