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

[Akim Demaille]

Le 3 juil. 2013 à 18:44, Valentin Tolmer <address@hidden> a écrit :

>> 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 pref number is higher than b's number.

I'm referring to the fact that currently precedences are
stated in increasing order:

%left "+"
%left "*"

means that * has higher precedence.  So I guess it would
rather translate in "+" < "*" with your system.

>> 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.

I would certainly make it easier to quickly grasp where things
start and end.  But ok.

>>> 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.

Don't do it this way.  You could use enums, that would be clearer.
Even two "==" and a "||" is more readable than a "& 1".

>>> 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.

Elsewhere, yes, not where the code is.

>>> +              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?

Why would you use a macro?  Avoid them, make it a function.


>>> /*---------------------------------.
>>> | 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.

However it's a pattern for structs that we introduce.
It makes the code easier to read.  Some day we might
even decide to move to C++; having constructors
does not sound wrong.

>>> @@ -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.

Bummer, you are right :)


>> 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.

OK.

>> 
>> Tests are needed!

I wanted to play with your patch, but to this end we really
need a branch on savannah.  Please, do that as soon as you
can.  Be sure not to touch to master, work on a separate branch.


> 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.

I have recently changed things in this area: when multiple
declarations are invalid, only the first one should be honored.
The others should be ignored, but, of course, generate errors.

> My patch fails when it tries to make the relation token > itself. So the 
> question is: is that an acceptable behavior?

Sorry, I don't understand what you mean here.

> 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).

I need examples :)

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