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

[nine . fierce . ballads]

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 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?

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 11:06:11, dak wrote:

> double x = dim_cache_[a].offset_ = 0.0;
> x would likely contain 1.0.

No, declaring the conversion operator "explicit" causes the compiler to
reject that.  In a situation where you actually wanted to convert like
that, you would need to cast.  Explicit conversion would also protect
you from

    double x = dim_cache_[a].offset_ + 1.0;

> bool x = dim_cache_[a].offset_ = 0.0;
> would result in a true value.

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

I agree that bool x = optional_something is a danger, but I don't think
you would be likely to type that unless you were trying to use
optional<bool>. That is one good reason to use something other than
optional<bool> when you want a tri-valued variable.

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_;
On 2018/07/07 11:06:11, dak wrote:
> Here we come to, uh, another learning experience.  The myopic view of
reviewers.
>   Our conversation could likely have benefited from the following
exchange:

Well, I'm sorry for my part of the misunderstanding too.  I'm sure I
read "used only once" as "used only in the dimension cache."

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 11:06:11, dak wrote:
> 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.

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 because they are
used in the standard library to make an object as if it had just been
initialized, but this leaves parent_ untouched.  It smells like
something is not quite right in the design here, but I'm trying very
hard not to branch out into any further refactoring in this area.

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];
On 2018/07/07 11:06:11, dak wrote:
> Not particularly happy about this kind of tame-the-const approach

This is a matter of habitual correctness, though in this case you won't
see the problems that omitting "mutable" could cause because no instance
of any kind of Grob is ever truly const (I partly assume).

In general, if no instance variables are declared mutable, the compiler
may place a const instance in unwritable memory, where your internal
implementation using const_cast has no hope of working as intended.  For
this reason, members that are changed in const methods should be
declared mutable.

Not having to use const_cast to modify those members blessed with
"mutable" then justifies approaching remaining appearances of const_cast
with suspicion.

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