Re: [RFC] Allow specification of semantic predicates.

[Paul Eggert]

On 07/22/10 19:17, Paul Hilfinger wrote:
> Comments appreciated on the following patch, intended for GLR parsers.

This looks like a nice thing to add, thanks!  I assume you still have
commit privileges?  I didn't look at the GLR implementation carefully,
but here are a few comments on the rest of it.

> +evaluated immediately (even when in nondeterministic mode).  If the 
> +expression yields a non-true value, the clause is treated as a syntax error,

That "yields a non-true value" grates.  More important, the
documentation doesn't say what happens when the value is nonzero.  I'd
change it to something like "If the expression yields a nonzero value,
it becomes the semantic value for that clause; otherwise, the clause
is treated as a syntax error."  Perhaps you can word this even better,
but I hope you get the idea.

> +actions in nondeterministic mode, since the latrer are deferred.

"latrer" -> "later".

> +this silently causes an alterative parse to die.  During deterministic

"alterative" -> "alternative"

> +Predicates actions, however, have no defined value, and may not be given

"Predicate actions", I guess?  Or did you mean "Predicates' actions"?

> +For example, trying to rewrite the previous example as 
> +
> address@hidden
> +widget :
> +          @{ if (!new_syntax) YYERROR; @} "widget" id new_args  @{ $$ = 
> f($3, $4); @}
> +       |  @{ if (new_syntax) YYERROR; @} "widget" id old_args   @{ $$ = 
> f($3, $4); @}
> +       ;
> address@hidden smallexample

The "(!new_syntax)" and "(new_syntax)" should be swapped here.

> -     fprintf (out, "b4_case(%d, [b4_syncline(%d, ", r + 1,
> -              rules[r].action_location.start.line);
> +     if (rules[r].is_predicate)
> +       fprintf (out, "b4_predicate_case(%d, [b4_syncline(%d, ", r + 1,
> +                rules[r].action_location.start.line);
> +     else
> +       fprintf (out, "b4_case(%d, [b4_syncline(%d, ", r + 1,
> +                rules[r].action_location.start.line);

Shouldn't the "if" be factored?  Something like this:

        fprintf (out, "b4_%scase(%d, [b4_syncline(%d, ", r + 1,
                 rules[r].is_predicate ? "predicate" : "",
                 rules[r].action_location.start.line);

> -<SC_BRACED_CODE>
> +<SC_BRACED_CODE,SC_PREDICATE>
>  {
>    "{"|"<"{splice}"%"  STRING_GROW; nesting++;
>    "%"{splice}">"      STRING_GROW; nesting--;
> +
> +  /* Tokenize `<<%' correctly (as `<<' `%') rather than incorrrectly
> +     (as `<' `<%').  */
> +  "<"{splice}"<"  STRING_GROW;
> +
> +  <<EOF>> {
> +    unexpected_eof (code_start, "}");
> +    STRING_FINISH;
> +    loc->start = code_start;
> +    val->code = last_string;
> +    BEGIN INITIAL;
> +    return BRACED_CODE;
> +  }
> +}

That "return BRACED_CODE" looks wrong.  Surely it should be something
like this:

    return YY_START == SC_BRACED_CODE ? BRACED_CODE : BRACED_PREDICATE;