Re: Allow (closed) scheme function calls as text scripts. (issue 6812088)

[dak]

On 2012/11/08 08:28:40, janek wrote:
> On 2012/11/05 22:45:06, thomasmorley65 wrote:
> > On 2012/11/05 22:40:36, lemzwerg wrote:
> > > Very nice!
> >
> > Can't review the code.
> > But from description:
> > GREAT

> The same in my case...
> David, could you add a comment (either to the parser code, or in the
commit
> message) that explains why the old code didn't work and how the new
code solves
> the problem?  Even if it's obvious for you, I think it would be
helpful for
> anyone reading and writing parser code in the future.

The problem with a code comment here is it would be more or less putting
lipstick on a pig.  The grammar consists of rules and alternatives,
partly dictated by functionality, partly by constraints.  The change
here was a single-word change, substituting embedded_scm_bare with
embedded_scm_closed.  If you wanted to know what embedded_scm_closed
means, you would look at the rule for it.  The rule states:

embedded_scm_closed:
        embedded_scm_bare
        | scm_function_call_closed
        ;

Which makes very much clear what the difference of embedded_scm_closed
as opposed to embedded_scm_bare is: it allows scm_function_call_closed
as one additional option.

Following that, you are lead to function_arglist_closed as next trivial
reference, and then there is a comment

 * function_arglist* / function_arglist_closed*: The closed variants
 * don't end in simple music expressions that might still accept
 * things like a duration or a postevent.

The closed expressions exist because of parser ambiguities and are
slated to go eventually, but while they are around, their meaning _is_
documented, and you get to the documentation by following the obvious
track.

The commit message might be misleading, however, since it sounds like it
is touting a fundamental new feature.  What this is is more like putting
another piece in place for the general idea of "strings/markups can be
submitted like any Scheme expression would".  It is more like plugging
an oversight rather than adding something new.  I had expected this
usage to have worked already before submitting this patch, and this
patch is supposed to make LilyPond match this expectation stemming from
earlier work apparently still incomplete.

So I might need to reword the commit message.

http://codereview.appspot.com/6812088/