lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Allow digits in identifiers (issue 6493072)


From: dak
Subject: Allow digits in identifiers (issue 6493072)
Date: Sun, 02 Sep 2012 11:52:46 +0000

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/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]