Re: [PATCH] grammar: new syntax allowing for partial order symbol precedence

[Valentin Tolmer]

Hi, thanks for the quick review.

> I confess I prefer using < in general, but I guess this way
> it makes your life easier when dealing with the handling of
> "<" and "<tag>".
I did not think about that.
> Note that current system presents precedences in the increasing
> order, the opposite of yours.
Not quite. In my system, if a > b, then the rule with a will be applied 
before b, and in the older one, a's prec number is higher than b's 
number.
>

> > @@ -263,66 +276,72 @@ resolve_sr_conflict (state *s, int ruleno, symbol 
> > **errors, int *nerrs)
> >    reductions *reds = s->reductions;
> >    /* Find the rule to reduce by to get precedence of reduction.  */
> >    rule *redrule = reds->rules[ruleno];
> > -  int redprec = redrule->prec->prec;
> > +  prec_node * redprecsym = redrule->prec->prec_node;
>
> prec_node *redprecsym
>
> >    bitset lookahead_tokens = reds->lookahead_tokens[ruleno];
> >
> >    for (i = 0; i < ntokens; i++)
> >      if (bitset_test (lookahead_tokens, i)
> > -        && bitset_test (lookahead_set, i)
> > -        && symbols[i]->prec)
> > +        && bitset_test (lookahead_set, i))
> >        {
> > -        /* Shift-reduce conflict occurs for token number i
> > -           and it has a precedence.
> > -           The precedence of shifting is that of token i.  */
> > -        if (symbols[i]->prec < redprec)
> > -          {
> > -            register_precedence (redrule->prec->number, i);
> > -            log_resolution (redrule, i, reduce_resolution);
> > -            flush_shift (s, i);
> > -          }
> > -        else if (symbols[i]->prec > redprec)
> > +        if (redprecsym && symbols[i]->prec_node)
> >            {
> > -            register_precedence (i, redrule->prec->number);
> > -            log_resolution (redrule, i, shift_resolution);
> > -            flush_reduce (lookahead_tokens, i);
> > +            /* Shift-reduce conflict occurs for token number i
> > +               and it has a precedence.
> > +               The precedence of shifting is that of token i.  */
> > +            if (is_prec_superior (redprecsym, symbols[i]->prec_node))
> > +              {
> > +                register_precedence (redrule->prec->number, i);
> > +                log_resolution (redrule, i, reduce_resolution);
> > +                flush_shift (s, i);
> > +              }
> > +            else if (is_prec_superior (symbols[i]->prec_node, redprecsym))
> > +              {
> > +                register_precedence (i, redrule->prec->number);
> > +                log_resolution (redrule, i, shift_resolution);
> > +                flush_reduce (lookahead_tokens, i);
> > +              }
> > +            else if (is_prec_equal (redprecsym, symbols[i]->prec_node))
> > +              /* Matching precedence levels.
> > +                 For non-defined associativity, keep both: unexpected
> > +                 associativity conflict.
> > +                 For left associativity, keep only the reduction.
> > +                 For right associativity, keep only the shift.
> > +                 For nonassociativity, keep neither.  */
> > +
> > +              switch (symbols[i]->assoc)
> > +                {
> > +                case undef_assoc:
> > +                  abort ();
> > +
> > +                case precedence_assoc:
> > +                  break;
> > +
> > +                case right_assoc:
> > +                  register_assoc (i, redrule->prec->number);
> > +                  log_resolution (redrule, i, right_resolution);
> > +                  flush_reduce (lookahead_tokens, i);
> > +                  break;
> > +
> > +                case left_assoc:
> > +                  register_assoc (i, redrule->prec->number);
> > +                  log_resolution (redrule, i, left_resolution);
> > +                  flush_shift (s, i);
> > +                  break;
> > +
> > +                case non_assoc:
> > +                  register_assoc (i, redrule->prec->number);
> > +                  log_resolution (redrule, i, nonassoc_resolution);
> > +                  flush_shift (s, i);
> > +                  flush_reduce (lookahead_tokens, i);
> > +                  /* Record an explicit error for this token.  */
> > +                  errors[(*nerrs)++] = symbols[i];
> > +                  break;
> > +                }
> > +            else
> > +              log_resolution (redrule, i, uncomparable_resolution);
>
> Wouldn't it be time to extract a small function in here?

Extracting a function would boil down to simply copy-paste the code out 
of the for loop, and passing it every local variable and argument, so I 
don't really think it would make the code simpler.
> > diff --git a/src/gram.h b/src/gram.h
> > index c1dd9a6..4dd2a88 100644
> > --- a/src/gram.h
> > +++ b/src/gram.h
> > @@ -117,6 +117,9 @@ typedef int item_number;
> > extern item_number *ritem;
> > extern unsigned int nritems;
> >
> > +/* Marker for the lexer and parser, to correctly interpret braces. */
> > +static int prec_braces = 0;
>
> What are its values?  Why the magic "& 1"?

I didn't know exactly how to do it, but the values for prec_braces are
0 -> default state
1 -> seen %gprec
3 -> seen %gprec name
4 -> inside the braces
hence the "& 1" to check if we are at the point of opening a brace.
> > diff --git a/src/scan-gram.l b/src/scan-gram.l
> > index 665e80d..f89647a 100644
> > --- a/src/scan-gram.l
> > +++ b/src/scan-gram.l
> > @@ -238,6 +238,11 @@ eqopt    ([[:space:]]*=)?
> >    "%param"                          RETURN_PERCENT_PARAM(both);
> >    "%parse-param"                    RETURN_PERCENT_PARAM(parse);
> >    "%prec"                           return PERCENT_PREC;
> > +  "%gprec" {
> > +    prec_braces = 1;
> > +    return PERCENT_GPREC;
> > +  }
>
> The indentation does not match the remainder of the code.
I don't see how : for every other case with braces blocks, the brace is 
right after the token and the code has one level of indentation.
> > +              to->symbol->tag, from->symbol->tag);
> > +  else if (is_prec_equal (from, to))
> > +    complain (&loc, fatal,
> > +              _("Contradicting declaration: %s > %s is in conflict with the \
> > +previous declaration of %s = %s"), from->symbol->tag, to->symbol->tag,
> > +              from->symbol->tag, to->symbol->tag);
>
> Using a common message with a %s for = or > would make translators lives
> easier.  Introduce a function to issue the error.
Is a macro OK?
> > /*---------------------------------.
> > | Create a new symbol, named TAG.  |
> > `---------------------------------*/
> > @@ -93,6 +345,12 @@ symbol_new (uniqstr tag, location loc)
> >    res->class = unknown_sym;
> >    res->status = undeclared;
> >
> > +  res->group_next = NULL;
> > +  res->prec_node = malloc (sizeof *res->prec_node);
> > +  res->prec_node->symbol = res;
> > +  res->prec_node->sons = NULL;
> > +  res->prec_node->equals = NULL;
>
> Shouldn't you introduce a prec_node_new function?
It's he only place where I create a prec_node, so I didn't deem it 
necessary.
> > @@ -541,11 +803,9 @@ symbol_check_alias_consistency (symbol *this)
> >    if (sym->prec || str->prec)
> >      {
> >        if (str->prec)
> > -        symbol_precedence_set (sym, str->prec, str->assoc,
> > -                               str->prec_location);
> > +        sym->assoc = str->assoc;
> >        else
> > -        symbol_precedence_set (str, sym->prec, sym->assoc,
> > -                               sym->prec_location);
> > +        str->assoc = sym->assoc;
>
> Use ?:.
I can't, it's not the same affectation.
> Should be a separated patch.  This patch is doing way too many
> things at the same time.

It slipped past my screening, I'll remove it.

> > +  sprintf (buff, "__anon%i__", anon_group_counter++);
>
> Don't use dangerous functions, use snprintf.  I'm not sure %i
> is portable.

How about %u? I'll make the counter unsigned.
> Tests are needed!
I'm working on them. This patch doesn't even pass all of bison's 
checks, I realised.
There is a problem with symbol redeclaration : how should I handle the 
precedence? So far, the attitude was to issue a warning and to 
overwrite the precedence level, I believe. My patch fails when it tries 
to make the relation token > itself. So the question is: is that an 
acceptable behaviour? If not, should I issue a warning and keep the 
precedence info as is? Or try to reconstitute the precedence as it 
would have been if the token had instead been declared there (that's 
going to be VERY hard, I'm afraid).

And I still have a few abort() to root out during the conflict 
resolution, but I think I know how to do that.