Restructure parsing of reverts to avoid ambiguities in relation to BACKUP

Restructure parsing of reverts to avoid ambiguities in relation to BACKUP

[janek, 2013-04-02 21:23:16]

I'm not sure what the previous ambiguities were, but since there's a big comment
above the code i guess that this should be reasonably easy to understand for
people knowledgeable in parser stuff.  So, LGTM.

[dak, 2013-04-02 21:40:04]

On 2013/04/02 21:23:16, janek wrote:
> I'm not sure what the previous ambiguities were

A music expression could start with BACKUP (an artificially inserted token never
present in the source code itself) due to these rules.  I needed BACKUP in a
different context and got shift/reduce conflicts because of these rules.  I
finally managed writing the _other_ code in a way reining its BACKUP tokens in,
so the respective patch could go on independent review/countdown.

It still makes sense, however, to recognize BACKUP only in the limited contexts
where it is actually being produced, in order to keep such conflicts down, and
to stop the parser from creative reinterpretation of situations that can only
arise by programmer error.

[janek, 2013-04-02 21:51:20]

Thanks fot the explanation!  Could you paste it into commit message?
Janek

On Tue, Apr 2, 2013 at 11:40 PM,  <dak@gnu.org> wrote:
> Reviewers: janek,
>
> Message:
>
> On 2013/04/02 21:23:16, janek wrote:
>>
>> I'm not sure what the previous ambiguities were
>
>
> A music expression could start with BACKUP (an artificially inserted
> token never present in the source code itself) due to these rules.  I
> needed BACKUP in a different context and got shift/reduce conflicts
> because of these rules.  I finally managed writing the _other_ code in a
> way reining its BACKUP tokens in, so the respective patch could go on
> independent review/countdown.
>
> It still makes sense, however, to recognize BACKUP only in the limited
> contexts where it is actually being produced, in order to keep such
> conflicts down, and to stop the parser from creative reinterpretation of
> situations that can only arise by programmer error.
>
> Description:
> Restructure parsing of reverts to avoid ambiguities in relation to
> BACKUP
>
> Please review this at https://codereview.appspot.com/8103043/
>
> Affected files:
>   M lily/parser.yy
>
>
> Index: lily/parser.yy
> diff --git a/lily/parser.yy b/lily/parser.yy
> index
>
ee413b301feb33f4d16d4a372c4a4c4c0ccfd8bc..795dcd27c953e062f6bffb4c317ada2f3bd84b18
> 100644
> --- a/lily/parser.yy
> +++ b/lily/parser.yy
> @@ -2066,6 +2066,13 @@ property_operation:
>  // \revert Accidental.color
>
>  revert_arg:
> +       revert_arg_backup BACKUP symbol_list_arg
> +       {
> +               $$ = $3;
> +       }
> +       ;
> +
> +revert_arg_backup:
>         revert_arg_part
>         {
>                 if (scm_is_null ($1)
> @@ -2074,20 +2081,16 @@ revert_arg:
>                 else
>                         MYBACKUP (SYMBOL_LIST, scm_reverse_x ($1, SCM_EOL),
> @1);
>         }
> -       | revert_arg BACKUP symbol_list_arg
> -       {
> -               $$ = $3;
> -       }
>         ;
>
>  // revert_arg_part delivers results in reverse
>  revert_arg_part:
>         symbol_list_part
> -       | revert_arg BACKUP SCM_ARG '.' symbol_list_part
> +       | revert_arg_backup BACKUP SCM_ARG '.' symbol_list_part
>         {
>                 $$ = scm_append_x (scm_list_2 ($5, $3));
>         }
> -       | revert_arg BACKUP SCM_ARG symbol_list_part
> +       | revert_arg_backup BACKUP SCM_ARG symbol_list_part
>         {
>                 $$ = scm_append_x (scm_list_2 ($4, $3));
>         }
>
>

[dak, 2013-04-02 22:22:38]

On 2013/04/02 21:51:20, janek wrote:
> Thanks fot the explanation!  Could you paste it into commit message?

Have you taken a look at the actual diff?  It is just factoring out one
subexpressions of revert into revert_arg_backup, making for about three trivial
changes.  Commit messages are cheap, but it seems like blowing the importance of
the patch way out of proportion.

[janek, 2013-04-02 22:56:31]

On 2013/04/02 22:22:38, dak wrote:
> On 2013/04/02 21:51:20, janek wrote:
> > Thanks fot the explanation! 
> > Could you paste it into commit message?
> 
> Have you taken a look at the actual diff? 

Of course.  I've also read the big comment above, but i didn't read the
remaining parser code and i'm totally unfamiliar with this code area.

> It is just factoring out one subexpressions of revert
> into revert_arg_backup, making for about three trivial
> changes.

Yes.  Unfortunately without better knowledge of parser code (I'm sorry, but i
cannot spend more than a few minutes reviewing each patch, so i cannot study the
code around carefully, and when i say that i'm unfamiliar with it i really mean
it) it wasn't obvious to me what the consequences of the change were.

> Commit messages are cheap, but it seems like blowing
> the importance of the patch way out of proportion.

Do as you wish.
Since the explanation is now already written (i mean, in your previous mail), i
think it would be a waste not to use it.  I guess that if it was completely
trivial, you wouldn't bother to write it ;) so it has some value.

Anyway, I'm obviously in favor of detailed commit messages, as can be seen here
https://codereview.appspot.com/7768043/

best,
Janek