Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

Dev mailing list
Good idea!  From inspection only, LGTM.

https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

Carl Sorensen
On 2020/02/11 21:46:52, lemzwerg wrote:
> Good idea!  From inspection only, LGTM.

Sounds like a great idea!

Carl


https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
Reviewers: lemzwerg, Carl,

Message:
On 2020/02/11 22:37:39, Carl wrote:
> On 2020/02/11 21:46:52, lemzwerg wrote:
> > Good idea!  From inspection only, LGTM.
>
> Sounds like a great idea!
>
> Carl

Well, I sort of got a feature request.
<https://music.stackexchange.com/questions/95259/lilypond-bug-with-octave-change-whole-note>

Description:
Special-case syntax error of duration before octave marks

A somewhat common note-entry error is to write something like c4''
that is (judging from my own experience) not even unusual for
experienced users to fall victim to occasionally.  So make a special
parser rule that deals "correctly" with that entry and delivers a more
helpful error message than the parser would muster on its own.

Please review this at https://codereview.appspot.com/557410043/

Affected files (+44, -0 lines):
  M lily/parser.yy


Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index 89511c5d9d67a8091cbf9e043d9b99def124be58..78f0269a55a4cb5999525b2d54c4aa03fdb2ea05 100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -3294,6 +3294,11 @@ quotes:
         | sup_quotes
         ;
 
+stray_quotes:
+ sub_quotes
+ | sup_quotes
+ ;
+
 sup_quotes:
  '\'' {
  $$ = scm_from_int (1);
@@ -3661,6 +3666,45 @@ pitch_or_music:
  $$ = n->unprotect ();
  }
  } %prec ':'
+ // Next rule is a frequent note entry error, like c4''
+ //
+ // It is quite unlikely that an octave check precedes a
+ // duration, but we have to keep it in the rule in order not
+ // to force the parser into early decisions before actually
+ // seeing a stray quote.  So we try to best interpret that
+ // case as well, even though it's not a likely error case.
+ | pitch exclamations questions octave_check duration stray_quotes optional_rest post_events {
+ if (!parser->lexer_->is_note_state ())
+ parser->parser_error (@1, _ ("have to be in Note mode for notes"));
+ {
+ Music *n = 0;
+ if (scm_is_true ($7))
+ n = MY_MAKE_MUSIC ("RestEvent", @$);
+ else
+ n = MY_MAKE_MUSIC ("NoteEvent", @$);
+
+ if (scm_is_number ($4))
+ {
+ int q = scm_to_int ($4) + scm_to_int ($6);
+ n->set_property ("absolute-octave", scm_from_int (q-1));
+ } else
+ $1 = unsmob<Pitch> ($1)->transposed
+ (Pitch (scm_to_int ($6), 0)).smobbed_copy ();
+
+ n->set_property ("pitch", $1);
+ n->set_property ("duration", $5);
+
+ if (to_boolean ($3))
+ n->set_property ("cautionary", SCM_BOOL_T);
+ if (to_boolean ($2) || to_boolean ($3))
+ n->set_property ("force-accidental", SCM_BOOL_T);
+ if (scm_is_pair ($8))
+ n->set_property ("articulations",
+ scm_reverse_x ($8, SCM_EOL));
+ $$ = n->unprotect ();
+ }
+ parser->parser_error (@6, _ ("octave marks must precede duration"));
+ } %prec ':'
  | new_chord post_events {
  if (!parser->lexer_->is_chord_state ())
                         parser->parser_error (@1, _ ("have to be in Chord mode for chords"));



Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

hanwenn
In reply to this post by Dev mailing list

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy
File lily/parser.yy (right):

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3676
lily/parser.yy:3676: | pitch exclamations questions octave_check
duration stray_quotes optional_rest post_events {
why can't this case be folded in to the preceding rule? Maybe this
comment says it already; if so an example would help.

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3706
lily/parser.yy:3706: parser->parser_error (@6, _ ("octave marks must
precede duration"));
add a comment that we sholudn't drop this error, even though this works
for users (I think we don't want that because it makes parsing .ly
harder for others.)

https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy
File lily/parser.yy (right):

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3676
lily/parser.yy:3676: | pitch exclamations questions octave_check
duration stray_quotes optional_rest post_events {
On 2020/02/12 06:45:08, hanwenn wrote:
> why can't this case be folded in to the preceding rule? Maybe this
comment says
> it already; if so an example would help.

Astute observation.  We print out the grammar in the manual and mixing
error productions and normal productions seems weird and misleading.
However, there is no way to hide error productions from the grammar, so
my version is not better in that regard (though stray_quotes seems
rather obvious).

But you are right: this calls for making folding into the previous rule.
Will do.

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3706
lily/parser.yy:3706: parser->parser_error (@6, _ ("octave marks must
precede duration"));
On 2020/02/12 06:45:08, hanwenn wrote:
> add a comment that we sholudn't drop this error, even though this
works for
> users (I think we don't want that because it makes parsing .ly harder
for
> others.)

Accepting that "syntax" would give me a rash.  I had enough of a problem
giving the compiler knowledge about it.  Having it accept it would make
me very unhappy.

I'll try to put in an appropriate comment.

https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

hanwenn
In reply to this post by Dev mailing list
looks OK.

Is there a way to exercise this? Maybe with #{ c4'' #} or something?

https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
On 2020/02/13 13:37:52, hanwenn wrote:
> looks OK.
>
> Is there a way to exercise this? Maybe with #{ c4'' #} or something?

Just { c4'' } would do it, but regtesting error messages is pretty icky.
 Warnings are more straightforward, but I really want an error.

https://codereview.appspot.com/557410043/

Reply | Threaded
Open this post in threaded view
|

Re: Special-case syntax error of duration before octave marks (issue 557410043 by dak@gnu.org)

hanwenn
In reply to this post by Dev mailing list