Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden)

[dak]

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/