Re: Issue 5368: Reduce allocations in Grob dimension caching (issue 359770043 by address@hidden)

[dak]

LGTM



https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc
File lily/grob.cc (left):

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#oldcode325
lily/grob.cc:325: *dim_cache_[a].offset_ += y;
This is more an "as you like it" suggestion: operator priority of * as
opposed to [] . -> is not always clear to everybody, so
*(dim_cache_[a].offset_) += y; might be a consideration.  On the other
hand, it's ugly.  And it's not like pointer-to-members operators .* and
->* which inexplicably have priority _less_ than * making for "in
theory, we can now write even more compound expressions without using
parens except that nobody will understand them except the compiler, so
we have to add more sanity parens than we otherwise would need to".

At any rate, I digress, sorry.  C++ language design is a topic getting
me into heavy breathing.  This was just to say "consider parenthising".
It's ok to consider and reject it.  In this case it makes the
expressions less similar, but they actually _are_ dissimilar, the former
working on the value container (filling it and marking it filled) and
giving a container expression as result, the latter working on the
container content and giving a double expression as result.

This similarity is part of the std::optional design and of course it's
not really the job of parens to change that.

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/359770043/diff/20001/lily/grob.cc#newcode322
lily/grob.cc:322: if (!dim_cache_[a].offset_.has_value ())
Actually, conversion to bool and direct assignment make for a bit of an
ugly combination in the std::optional design: if I read the docs
correctly, after

double x = dim_cache_[a].offset_ = 0.0;

x would likely contain 1.0.  Or, if explicit conversion intervenes, at
least

bool x = dim_cache_[a].offset_ = 0.0;

would result in a true value.

So particularly since we are using offset_ here in numerical context, I
am more amenable to your use of has_value rather than bool conversion
here.  It's too easy to forget the * operator and we do test doubles for
being zero by just using them as a boolean in several places.

So independent of the whole business of having a separate optional class
or not, for this particular use I consider the explicit use of has_value
that you do here an improvement.

https://codereview.appspot.com/359770043/diff/20001/lily/include/dimension-cache.hh
File lily/include/dimension-cache.hh (right):

https://codereview.appspot.com/359770043/diff/20001/lily/include/dimension-cache.hh#newcode62
lily/include/dimension-cache.hh:62: Optional<Real> offset_;
Here we come to, uh, another learning experience.  The myopic view of
reviewers.  Our conversation could likely have benefited from the
following exchange:

    I see no point in a templated class for something that is used only
once.

    Ah, but it is used for Real as well as Interval, independent uses
though both within the dimension-cache framework.

    Oh.  I see.  I have overlooked this.

Now I'm fine with pulling this limited-use templated class into the
dimension cache definition for now where we know that it matches the
requirements of the particular uses here.  And I'd suggest just sticking
with it for now.  I think it's a good solution even though the review
process was tarnished by me not realizing what we were actually talking
about.

https://codereview.appspot.com/359770043/diff/20001/lily/include/dimension-cache.hh#newcode72
lily/include/dimension-cache.hh:72: void clear ()
Is there some incentive for "clear" over "reset"?  If it conceptually
matches what "reset" for the elements does (does it?), it may make sense
matching the naming as well.

https://codereview.appspot.com/359770043/diff/20001/lily/include/grob.hh
File lily/include/grob.hh (right):

https://codereview.appspot.com/359770043/diff/20001/lily/include/grob.hh#newcode44
lily/include/grob.hh:44: mutable Dimension_cache dim_cache_[NO_AXES];
Not particularly happy about this kind of tame-the-const approach, but
it's already in paper-score code from 2010 so it's not exactly new.

https://codereview.appspot.com/359770043/