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

[dak]

Still 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;
On 2018/07/07 14:48:27, Dan Eble wrote:
> On 2018/07/07 11:06:11, dak wrote:
> > 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

> I would have no problem adding parentheses, but how do you rate the
beauty of
> using value_or() here as in Patch Set 1?

Reads clumsy to me, though it's basically the same as our robust_scm2* .
 Let's just leave this one alone for now.

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 ())
On 2018/07/07 14:48:27, Dan Eble wrote:

> Well, even bool x = 0.0 would cause me to question the state of mind
of the
> programmer.

lily/bar-check-iterator.cc:      if (where->main_part_)

Pretty much the same.  We are not using all that many inexact numbers in
a similar manner I guess but numerics used as conditions are not that
infrequent although it's admittedly more integers (like size ()) used in
that manner.

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#newcode72
lily/include/dimension-cache.hh:72: void clear ()
On 2018/07/07 14:48:27, Dan Eble wrote:

> It was already called clear () and I was trying not to change more
than I had
> to.

> In this case I don't really like either of those names

So just not wanting to condone either and not touching anything.

Ok with me.

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