Reviewers: dak, Message: Thank you for your review. I disagree with some of your conclusions.
https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc File lily/acceptance-set.cc (right): https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc#newcode34 lily/acceptance-set.cc:34: if (!scm_is_null (default_)) On 2018/06/14 22:53:58, dak wrote:
Why do you prioritize the default child?
I don't think I can take credit for that, though maybe I can take blame for misunderstanding Context_def::get_accepted in the old code. Doesn't it place the default child at the front of the list that Context_def::internal_path_to_acceptable_context () considers? That would make the default child the preferred alternative when \context searches for an instantiable child. At least, that is how I read it. https://codereview.appspot.com/346080043/diff/1/lily/acceptance-set.cc#newcode50 lily/acceptance-set.cc:50: accepted_ = scm_cons(item, scm_delete_x (item, accepted_)); On 2018/06/14 22:53:58, dak wrote:
I'm not overly happy with this kind of O(n^2) behavior. Most of the
context
definitions will be done in engraver-init.ly where duplication is not
an issue
because it has been taken care of manually.
Seeing that engraver-init.ly had at most a couple dozen accepts in a context definition, my gut feeling was that it would be better to default to cleanliness rather than worry about scaling, but I have no problem with going back to allowing duplicate entries and mentioning it in a comment to discourage the next person who comes along. https://codereview.appspot.com/346080043/diff/1/lily/context-def.cc File lily/context-def.cc (right): https://codereview.appspot.com/346080043/diff/1/lily/context-def.cc#newcode330 lily/context-def.cc:330: Acceptance_set& acc = context->acceptance_; On 2018/06/14 22:53:58, dak wrote:
You take the original list without copying and call
context->acceptance_ = acceptance_ (line 328) copies the acceptance set from the Context_def to the Context and then the following code modifies the Context's acceptance set. I could use a named method instead of the assignment operator to make this clearer.
Previously, \accept/\denies/\defaultchild for better or worse were not interpreted
in \with
blocks (which is where ops comes from if I remember correctly).
\accepts was. \denies and \defaultchild were not until I added them a couple weeks ago: https://codereview.appspot.com/346050043/ If any of that is a problem, let me know and I will prepare a patch to revert what you please; I don't understand why it would be a problem, though. Anyway, in the old version of Context::path_to_acceptable_context, these mods were explicitly looped over, converted from strings to symbols, passed into Context_def::path_to_acceptable_context and merged into a fresh copy of the Context_def's list of accepted contexts. This was repeated for every call to Context::path_to_acceptable_context, which occurred potentially multiple times during a find-or-create walk of the context hierarchy. In the new code, the Context's acceptance list is computed here, once, when the context is instantiated.
lets \with blocks destructively change global context defs.
I do not think this is the case, but I will verify it. Description: https://sourceforge.net/p/testlilyissues/issues/5344/ Encapsulate the list of accepted contexts into Acceptance_set, which is owned by both Context_def and Context. The merging of context mods that used to occur anew for every path_to_acceptable_context () call now occurs once when a context is instantiated. The aspect of this change most in need of review is probably garbage collection. Thanks in advance. Please review this at https://codereview.appspot.com/346080043/ Affected files (+188, -125 lines): A lily/acceptance-set.cc M lily/context.cc M lily/context-def.cc M lily/global-context.cc A lily/include/acceptance-set.hh M lily/include/context.hh M lily/include/context-def.hh _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
