Re: [PATCH] symbolic names

[Akim Demaille]

Le 29 mars 09 à 23:09, Alex Rozenman a écrit :

> Hi Akim, Joel.

Hi Alex,

Please, also keep us informed about the legal paperwork dependencies.   
I'm eager to see your changes checked in.  In the meanwhile, could you  
please remove the generated files from your posted diffs?  Thanks in  
advance!

> A new version is attached. I just implemented most of the proposals  
> from our previous thread. You can also see fragments of bison output  
> messages below.  Meanwhile, I added some tests to the suite, but  
> we'll need much more testing - there are tens possible combinations  
> of this stuff. You are welcome to try it :)
>
> (ordinary ambiguity)
> if_stmt1: IF expr[cond] THEN stmt ELSE stmt FI
>            { $if_stmt1 = new IfStmt($cond, $stmt, $stmt); };
> interpreter.ypp:79.44-48: reference is ambiguous: `$stmt'
> interpreter.ypp:78.30-33:   refers to: $stmt at $4
> interpreter.ypp:78.40-43:   refers to: $stmt at $6
> interpreter.ypp:79.51-55: reference is ambiguous: `$stmt'
> interpreter.ypp:78.30-33:   refers to: $stmt at $4
> interpreter.ypp:78.40-43:   refers to: $stmt at $6

I like this, and I prefer to have alternatives aligned, it's easier to  
see the differences.  Do you always display $, or display @ when it is  
the culprit?  What about crossed ambiguities, say

if_stmt1: IF expr[cond] THEN stmt ELSE stmt FI
           { $if_stmt1 = new IfStmt(@cond, $cond,
                                    @stmt, $stmt,
                                    @stmt, $stmt); };

> (former "assymetric renaming")
> if_stmt1: IF expr[cond] THEN stmt[then] ELSE stmt[else] FI
>            { $if_stmt1 = new IfStmt($cond, $then, $stmt); };
> interpreter.ypp:79.51-55: reference is misleading: `$stmt'
> interpreter.ypp:78.30-33:   possibly meant: $stmt at $4, symbol hidden
> interpreter.ypp:78.46-49:   possibly meant: $stmt at $6, symbol hidden

Hum...  I don't think it is misleading, it is plain "wrong".  The name  
"stmt" is not available at all.  And in the "possibly meant", we  
should probably display the right name.  This might be asking for too  
much in the implementation, I don't know.

> interpreter.ypp:79.51-55: identifier is hidden: `$stmt'
> interpreter.ypp:78.30-33:   possibly meant: $then at $4
> interpreter.ypp:78.46-49:   possibly meant: $else at $6

(Note that we are sometimes using `', and sometimes not.  Here, since  
there can never be surprises such as spaces, commas and colons in the  
names, I think we can get rid of them).

> (reference to a symbol with dots)
> if_stmt: IF expr[cond] THEN stmt.list FI
>            { $if_stmt = new IfStmt($cond, $stmt.list, 0); };
> interpreter.ypp:76.43-52: reference not found: `$stmt.list'

I don't understand this one.  What is not found is $stmt.

> interpreter.ypp:75.29-37:   possibly meant: $[stmt.list] at $4

> (refernce to a field)
> if_stmt: IF expr[cond] THEN stmt FI
>            { $if_stmt = new IfStmt($cond, $stmt.list, 0); };
> <no errors>

Fine :)


> (reference to a symbol with dots and a field)
> if_stmt: IF expr[cond] THEN stmt.list FI
>            { $if_stmt = new IfStmt($cond, $stmt.list.f1, 0); };
> interpreter.ypp:76.43-55: reference not found: `$stmt.list.f1'

Again, I would only refer to $stmt.

> interpreter.ypp:75.29-37:   possibly meant: $[stmt.list].f1 at $4



> (bracketed syntax for a symbol with dots)
> if_stmt: IF expr[cond] THEN stmt.list FI
>            { $if_stmt = new IfStmt($cond, $[stmt.list].f1, 0); };
> <no errors>

Fine.


> (access from mid-rule action)
> if_stmt: IF expr[cond] THEN { $if_stmt = $then; } stmt.list[then]  
> ELSE stmt.list[else] FI
>   { $if_stmt = new IfStmt($cond, $then, $else); };
> interpreter.ypp:78.31-38: reference not found: `$if_stmt'
> interpreter.ypp:78.10-79.49:   possibly meant: $if_stmt at $$,  
> cannot be accessed from mid-rule action at $4
> interpreter.ypp:78.42-46: reference not found: `$then'
> interpreter.ypp:78.61-64:   possibly meant: $then at $5, cannot be  
> accessed from mid-rule action at $4

Perfect!  I guess this is much better than what we tell the user when  
she is using only numeric references.

> (ordinary "not found")
> if_stmt1: IF expr[cond] THEN stmt[then] ELSE stmt[else] FI
>            { $if_stmt1 = new IfStmt($cond1, $[then], $else); };
> interpreter.ypp:82.37-42: reference not found: `$cond1'

Perfect.



I'm not sure "reference not found" is right.  I understand "undeclared  
identifier", "duplicate definition of identifier", "reference cannot  
be solved" etc., but I'm not sure that "to find a reference" is right.



In the patch, using 2009 as copyright years, FSF projects pay a lot of  
attention to this.  Sometimes you have just 2000, and sometimes many  
years (but 2009 :) although the file is new.


Sometimes you write "named_ref* named_ref", it should be "named_ref  
*named_ref". Using the same identifier for types and variables is new  
in Bison I think, but it looks nice in your code.

I'm surprised to see that `.' belongs to your 'letter' set of  
characters, so you really scan '\$[a-zA-Z_.]+' as named references.   
But this might be done to help reporting detailed errors to the user.

Be sure to leave braces alones on their own lines (find_prefix_end is  
wrong).

'\0' looks nicer that (char)0, but 0 alone should be ok, should it not?

In variant_add, I think you can fuse the two if's.

+  if (('0' <= *cp && *cp <= '9') || *cp == '-')

I believe we avoid relying on the encoding.  Use isdigit instead.

Please, use snprintf for at_buf.  I suppose we will never overflow  
this buffer, still this makes me nervous.

message_buf is more of a problem, you can't expect to know the size of  
the translation. Something else must be found here.  Maybe obstracks  
are your friends here.

Please, refrain for declaring several identifiers on a single type, in  
particular like here:

+  char *cp = text + 1, *gt_ptr = 0;

Please, stay with 76 columns (80 max).  Problems in scan-gram.l.

Wouldn't it be simpler, in scan-gram.l, to read [id] in a single block  
instead of first [, then {id}, and finally ]?  It looks to me that you  
have move error cases to consider in your case.

We should really factor the scanner for the calculators in the test  
suite...


Great job!