This reply is not intended to refute your contention that there are too few comments. It should rather make you understand what is actually going on so that you can improve the suggestions. Maybe even more documentation elsewhere would be needed.
https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc File lily/music-function.cc (right): https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode63 lily/music-function.cc:63: bool skipping = false; On 2015/06/08 04:36:54, Keith wrote:
The 'skipping' flag and the early continues are hard to follow. We
want to keep
this logic synchronized with the duplicate functionality in the
parser, so
either now or later it would be good to make the logic more clear.
What would be clear for me is a boolean remembering whether the
argument
currently being matched is optional:
It doesn't do the trick. Optional arguments following an unmatched optional argument are automatically unmatched themselves. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode83 lily/music-function.cc:83: skipping = true; On 2015/06/08 04:36:54, Keith wrote:
Here we failed to type-match an optional argument, and will fill the
slot with
the default value on line 88, so why set the 'skipping' flag ? If
there are two
optional parameters in a row, the 'skipping' flag would seem to force
the next
entry in the signature to be filled with its default value as well.
This is _exactly_ what it does. You say "It bothered me that I said 'LGTM' without figuring out the logic. I don't understand it. We want to keep this logic synchronized with the duplicate functionality in the parser," I rather think you don't understand what the parser does. Which is hardly surprising since the parser logic is written in a manner where the rules are applied from last argument to first with the predicates coming in in reverse order and the actual tokens being read front to back. I've managed to get it streamlined somewhat over the years but it made my head hurt for weeks to get the basic control logic for optional arguments and their skipping correct. The achieved functionality is exacly what is done here. Only that this code here does it in a straightforward manner speaking for itself with the semantics clearly in the open. Given the comparative complexity of the implementation in an LALR(1) parser, I might be guilty of "simple enough to not need a comment" syndrome here. But if you are confused because this code seems to do something different than the parser does, chances are very much that it is because you understand what this code does and misunderstand how the parser does the same. Which is absolutely understandable. The only excuse for the parser doing this job as complex as it does is that it cannot really be done any simpler using the parser generator we use, and once it works, it's comparatively low-maintenance. So your problem appears to be that you understand the code but it does something you did not expect. Cross-check from the user end with the "Extending LilyPond" guide's explanation of optional arguments. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode88 lily/music-function.cc:88: args = scm_cons (with_loc (scm_cdr (pred), location), args); On 2015/06/08 04:36:54, Keith wrote:
For an optional argument with no default value, it seems we would want
#f, but
scm_cdr will return the empty list.
Signatures are not stored in the original form. They are slightly processed in order to make their use faster. scm_cdr will return #f since (number? . #f) is what will be stored for a predicate written as (number?) while (number? . 7) is what will be stored for (number? 7). Could be worth mentioning. https://codereview.appspot.com/244840043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
