Re: Syntax: lyric_mode_music might be a MUSIC_FUNCTION (issue 343820043 by address@hidden)

[dak]

On 2018/04/25 06:33:19, knupero wrote:
> Please test & review the proposed syntax change.

I don't see the point in starting a formal review after this has already
been discussed on the mailing list.  In particular since this differs
from your originally proposed patch (where you did not switch to lyrics
mode at all) and you disengenuously continued the discussion based on a
modified patch (the one proposed here) which you then tacked onto your
results without further mention.  Now let's review this modified patch.
The respective passage in the parser contains the comment

// We must not have lookahead tokens parsed in lyric mode.  In order
// to save confusion, we take almost the same set as permitted with
// \lyricmode and/or \lyrics.  However, music identifiers are also
// allowed, and they obviously do not require switching into lyrics
// mode for parsing.

Your code gets out of lyrics mode after scanning the music function
_and_ a prospective lookahead token.  This lookahead token will
consequently be parsed in the wrong state (lyrics mode instead of music
mode).  For example, in

{
  e1 \addlyrics \displayLilyMusic hi-there
  c1
}

c1 will not be recognized as a note event since it is parsed in lyrics
mode as hi-there could be followed by a post-event.  Most music function
arguments will be terminated by examining a lookahead token.  A braced
list (as used in your examples) is one of the rare kind of arguments
that isn't.

We don't state "please don't use that syntax in the following way or
LilyPond will be silently confused into producing nonsense" in other
respects so it is irrelevant that LilyPond can parse _some_ input
correctly with that change and will "only" get confused about other
input.

https://codereview.appspot.com/343820043/