Re: Make music functions callable from Scheme (issue 244840043 by address@hidden)

[dak]

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/