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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
