All in all, a large step backwards for making the lexer behave predictably across modes regarding word and command syntax, connected with several timing problems when parsing.
It looks like some _severe_ doctoring around with regard to notenames was done to make regtests pass without an actual understanding of the failure modes, introducing some half-baked in-between modes that don't have a purpose apart from papering over the fact that this patch causes the lexer to be in the wrong mode due to parser lookahead at several points of time. Regtests that showed themselves to be impervious to this papering over got some gratuitous braces added until passing. This is more a demonstration how to game our automated patch submission and regtest evaluation system than a serious proposal. http://codereview.appspot.com/6493072/diff/14/input/regression/page-spacing-nonstaff-lines-independent.ly File input/regression/page-spacing-nonstaff-lines-independent.ly (left): http://codereview.appspot.com/6493072/diff/14/input/regression/page-spacing-nonstaff-lines-independent.ly#oldcode11 input/regression/page-spacing-nonstaff-lines-independent.ly:11: \addlyrics { high \skip2 } Clear case of "existing use". In previous syntax discussions, there was some agreement on formatting durations to follow directly their note, like c4. \skip2 would be consistent with that. It is also not clear why c5 should not be a valid identifier when ce5 is. http://codereview.appspot.com/6493072/diff/14/input/regression/phrasing-slur-multiple.ly File input/regression/phrasing-slur-multiple.ly (left): http://codereview.appspot.com/6493072/diff/14/input/regression/phrasing-slur-multiple.ly#oldcode21 input/regression/phrasing-slur-multiple.ly:21: c4\(\(\sp1\( d4\)\(\sp1\( e4\) f\) | This particular way of calling things is probably not important enough to preserve, but \sp1 is basically a modifier on the following \( and the spacier formatting proposed in the change does not have quite the same expressive power. http://codereview.appspot.com/6493072/diff/14/input/regression/ragged-bottom-one-page.ly File input/regression/ragged-bottom-one-page.ly (right): http://codereview.appspot.com/6493072/diff/14/input/regression/ragged-bottom-one-page.ly#newcode13 input/regression/ragged-bottom-one-page.ly:13: \repeat unfold 16 { c'4 } Why are the braces needed here? http://codereview.appspot.com/6493072/diff/14/input/regression/skiptypesetting-show-first-and-last.ly File input/regression/skiptypesetting-show-first-and-last.ly (right): http://codereview.appspot.com/6493072/diff/14/input/regression/skiptypesetting-show-first-and-last.ly#newcode11 input/regression/skiptypesetting-show-first-and-last.ly:11: showLastLength = { R1*2 } And here? http://codereview.appspot.com/6493072/diff/14/lily/include/lily-lexer.hh File lily/include/lily-lexer.hh (right): http://codereview.appspot.com/6493072/diff/14/lily/include/lily-lexer.hh#newcode101 lily/include/lily-lexer.hh:101: void push_maybe_note_state (); Uh oh... "maybe_note_state" sounds like a real can of worms. http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll File lily/lexer.ll (left): http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll#oldcode397 lily/lexer.ll:397: <chords,notes,figures>{RESTNAME}/[-_] | // pseudo backup rule Did you check that r-. does still work as intended when removing this rule? http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll File lily/lexer.ll (right): http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll#newcode34 lily/lexer.ll:34: flex -b <this lexer file> Did you do this? http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll#newcode476 lily/lexer.ll:476: {A}+ { So words no longer correspond to commands regarding their syntax in note mode? Doesn't that mean that things like \layout { \tempo "Allegro" some-variable = 17 } stop working again? http://codereview.appspot.com/6493072/diff/14/lily/lexer.ll#newcode859 lily/lexer.ll:859: push_note_state (nn); What is this supposed to do? INITIAL state is not supposed to have pitchnames defined. http://codereview.appspot.com/6493072/diff/14/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/6493072/diff/14/lily/parser.yy#newcode1190 lily/parser.yy:1190: '{' { parser->lexer_->push_maybe_note_state (); } music_list '}' Pushing a separate "maybe_note_ state for _every_ braced music list? Seriously? http://codereview.appspot.com/6493072/diff/14/lily/parser.yy#newcode1834 lily/parser.yy:1834: } function_arglist { This spells the death knell for having functions decide on their syntactic role based on their _return_ value when every music function needs to get parsed in a special state. http://codereview.appspot.com/6493072/diff/14/lily/parser.yy#newcode1837 lily/parser.yy:1837: parser->lexer_->pop_state (); You are aware that in general a music function call boundary will be determined by looking at a lookahead token? That means that the lookahead token will, in general, have been parsed in the wrong lexer state. Since your changes introduce gratuitious changes between word syntax based on lexer state, this will generally cause trouble. Probably the reason you had to introduce braces in some regtests. http://codereview.appspot.com/6493072/diff/14/lily/parser.yy#newcode2218 lily/parser.yy:2218: parser->lexer_->pop_state (); Again, likely too late. You should pop state before ANGLE_CLOSE to be on the safe side, though in this particular case the parser might well switch state without the need of a lookahead token. http://codereview.appspot.com/6493072/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
