In summary: at the current point of time the added complexity because of the employed constructs does not make the impression of being a good tradeoff for understanding and maintaining the code.
I think you need to define the goals you want to achieve, then focus on the simplest way of implementing a version in line with your goals. The current version uses considerably more complex code and constructs than the previous version without achieving a tangible goal in that manner. If you want to unify code, farming out into 9 separate function template instantiations that could be individually retemplated is a debugging complication for the sake of rather minimal constant expression/deadcode elimination (we are talking about gains here that don't offset a single additional call of Music::origin by far). NLGTM https://codereview.appspot.com/344050043/diff/1/lily/context-specced-music-iterator.cc File lily/context-specced-music-iterator.cc (right): https://codereview.appspot.com/344050043/diff/1/lily/context-specced-music-iterator.cc#newcode44 lily/context-specced-music-iterator.cc:44: Input *origin = get_music ()->origin (); Personally, I'd not get this in advance. It is only needed when warnings are emitted. And in that case, saving one access for each of at least two warnings is irrelevant. https://codereview.appspot.com/344050043/diff/1/lily/context-specced-music-iterator.cc#newcode63 lily/context-specced-music-iterator.cc:63: a = a->check_access (origin); What is this for? Comment? In the spirit of avoiding unnecessary access of "origin", could we pass something else here? The music maybe? Would that be in line with possible other uses of check_access? https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc File lily/global-context.cc (right): https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc#newcode129 lily/global-context.cc:129: return get_score_context (); If I understand correctly, this will cause different behavior depending on whether or not a Score context has already been created. https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc#newcode143 lily/global-context.cc:143: return score; So if there already is a Score context, it will be returned in lieu of _any_ request for creating any kind of context? That sounds weird. \new Score \context Global \context Voice << c d >> would create two different Voice contexts since \context Voice is remapped to \context Score ? What is that good for? Or am I completely mistaken here? https://codereview.appspot.com/344050043/diff/1/lily/include/context.hh File lily/include/context.hh (right): https://codereview.appspot.com/344050043/diff/1/lily/include/context.hh#newcode61 lily/include/context.hh:61: template <FindMode mode, Direction dir> This creates a family of 9 different functions for each of the two functions you include here. Since the dir argument is actually being read from the command in question rather than being fixed at compile time, you then need to hardcode a farmout to the various variants since the actually called variant cannot, after all, be determined at compile time in general. That's a lot of complication for no discernible gain. Why not pass the respective parameters into the function? Would that cause some kind of problem? https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc File lily/change-iterator.cc (right): https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc#newcode67 lily/change-iterator.cc:67: Context::warning_cannot_find (origin, to_type, to_id); Having a separate function for each different warning text seems clumsy. Can this be factored into a more generic warning function and possibly something formatting to_type and to_id? https://codereview.appspot.com/344050043/diff/20001/lily/context.cc File lily/context.cc (right): https://codereview.appspot.com/344050043/diff/20001/lily/context.cc#newcode250 lily/context.cc:250: Context::find<Context::FIND_ONLY, UP> (SCM, const string &, SCM); The cure seems worse than the ailment. https://codereview.appspot.com/344050043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
