Re: parser.yy: remove STRING_IDENTIFIER token (issue 6542057)

[dak]

Reviewers: lemzwerg, joeneeman, janek,

Message:
On 2012/09/26 05:35:16, janek wrote:
> David wrote:
> > ...and where the semantics indeed require a plain string,
> > an error is generated explicitly in the action instead
> > of leaving it to the parser to complain about bad grammar.

> I'm not sure i understand.  Does this mean that in some situations
there will be
> errors that were not present before this change?  I.e, will something
stop
> working?

No.  The error messages and recovery will just be different.  When the
parser proper detects something that it can't use in its current state,
it throws a cryptic error message and starts throwing material away
until it thinks it can reasonably recover.

After the change, the parser proper accepts non-trivial markups where
only a string would be allowed, and the code executed on behalf of the
parser will throw the error instead without aborting.

In short, instead of a cryptic error message and dubious error recovery,
the user gets a human-understandable error message and a recovery more
likely not to cause follow-up errors.

This is for the case where things were working fine previously.  As
mentioned in the issue description, there were several situations where
things broke totally counterintuitively (namely where markups were
accepted only when they consisted of non-trivial strings, but plain
strings were rejected).

In other words: after contemplating your review, I don't see that the
countdown should be affected, but if you want to suggest a rewording of
the commit message soonish, I can of course incorporate it.

A code comment to that effect does not make all that much sense in
comparison since there is little point in writing a comment for the
replacement of code that is not even there anymore when the replacement
is, in itself, quite self-explanatory.

Description:
parser.yy: remove STRING_IDENTIFIER token

A STRING_IDENTIFIER always was a special case of a MARKUP_IDENTIFIER
without being treated equally at top level.  That led to craziness
like

xxx=\markup "test"
\xxx

being invalid, while

xxx=\markup \italic "test"
\xxx

was valid code.  This change just removes the STRING_IDENTIFIER
category altogether.  Where it was used previously, MARKUP_IDENTIFIER
is accepted now syntactically, and where the semantics indeed require
a plain string, an error is generated explicitly in the action instead
of leaving it to the parser to complain about bad grammar.

Please review this at http://codereview.appspot.com/6542057/

Affected files:
  M lily/parser.yy


Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index  
94227e8715ee5c72ac796ee00b1d65284539fa9a..52640be0b80cfdf9f6722753e04d4d88df55c238  
100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -380,7 +380,6 @@ If we give names, Bison complains.
 %token SCM_TOKEN
 %token SCORE_IDENTIFIER
 %token STRING
-%token STRING_IDENTIFIER
 %token TONICNAME_PITCH

 %left '-' '+'
@@ -473,7 +472,6 @@ embedded_scm_bare:
 embedded_scm_bare_arg:
        embedded_scm_bare
        | STRING
-       | STRING_IDENTIFIER
        | full_markup
        | full_markup_list
        | context_modification
@@ -576,7 +574,6 @@ identifier_init:
        | FRACTION
        | string
         | embedded_scm
-       | full_markup
        | full_markup_list
         | context_modification
        ;
@@ -1859,10 +1856,16 @@ string:
        STRING {
                $$ = $1;
        }
-       | STRING_IDENTIFIER {
-               $$ = $1;
-       }
+       | full_markup
        | string '+' string {
+               if (!scm_is_string ($1)) {
+                       parser->parser_error (@1, (_ ("simple string 
expected")));
+                       $1 = scm_string (SCM_EOL);
+               }
+               if (!scm_is_string ($3)) {
+                       parser->parser_error (@3, (_ ("simple string 
expected")));
+                       $3 = scm_string (SCM_EOL);
+               }
                $$ = scm_string_append (scm_list_2 ($1, $3));
        }
        ;
@@ -1873,8 +1876,23 @@ simple_string: STRING {
        | LYRICS_STRING {
                $$ = $1;
        }
-       | STRING_IDENTIFIER {
-               $$ = $1;
+       | MARKUP_IDENTIFIER
+       {
+               if (scm_is_string ($1)) {
+                       $$ = $1;
+               } else {
+                       parser->parser_error (@1, (_ ("simple string 
expected")));
+                       $$ = scm_string (SCM_EOL);
+               }
+       }
+       | LYRIC_MARKUP_IDENTIFIER
+       {
+               if (scm_is_string ($1)) {
+                       $$ = $1;
+               } else {
+                       parser->parser_error (@1, (_ ("simple string 
expected")));
+                       $$ = scm_string (SCM_EOL);
+               }
        }
        ;

@@ -2309,7 +2327,13 @@ gen_text_def:
                t->set_property ("text", $1);
                $$ = t->unprotect ();
        }
-       | simple_string {
+       | STRING {
+               Music *t = MY_MAKE_MUSIC ("TextScriptEvent", @$);
+               t->set_property ("text",
+                       make_simple_markup ($1));
+               $$ = t->unprotect ();
+       }
+       | LYRICS_STRING {
                Music *t = MY_MAKE_MUSIC ("TextScriptEvent", @$);
                t->set_property ("text",
                        make_simple_markup ($1));
@@ -2974,9 +2998,6 @@ simple_markup:
        | LYRIC_MARKUP_IDENTIFIER {
                $$ = $1;
        }
-       | STRING_IDENTIFIER {
-               $$ = $1;
-       }
        | SCORE {
                SCM nn = parser->lexer_->lookup_identifier ("pitchnames");
                parser->lexer_->push_note_state (nn);
@@ -3032,10 +3053,7 @@ otherwise, we have to import music classes into the  
lexer.
 int
 Lily_lexer::try_special_identifiers (SCM *destination, SCM sid)
 {
-       if (scm_is_string (sid)) {
-               *destination = sid;
-               return STRING_IDENTIFIER;
-       } else if (unsmob_book (sid)) {
+       if (unsmob_book (sid)) {
                Book *book =  unsmob_book (sid)->clone ();
                *destination = book->self_scm ();
                book->unprotect ();