Re: [PATCH] symbolic names

[Alex Rozenman]

Hi Akim,

 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); };

interpreter.ypp:57.36-40: reference is ambiguous: address@hidden'
interpreter.ypp:55.30-33:   refers to: @stmt at $4
interpreter.ypp:55.40-43:   refers to: @stmt at $6
interpreter.ypp:57.43-47: reference is ambiguous: `$stmt'
interpreter.ypp:55.30-33:   refers to: $stmt at $4
interpreter.ypp:55.40-43:   refers to: $stmt at $6


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

The current model is to show the symbol that actually was referenced and to
explain why it cannot be referenced. It works the same way with bracketing
and mid-rule action visibility.
In the following case:
if_stmt1: IF expr[cond] THEN stmt[then] ELSE stmt.list[else] FI
          { $if_stmt1 = new IfStmt($cond, $stmt.x, $stmt.list); };

Current output:
interpreter.ypp:56.43-49: reference is invalid: `$stmt.x'
interpreter.ypp:55.30-33:   possibly meant: $[stmt].x at $4, symbol hidden
interpreter.ypp:56.52-61: reference is misleading: `$stmt.list'
interpreter.ypp:55.30-33:   possibly meant: $[stmt].list at $4, symbol
hidden
interpreter.ypp:55.46-54:   possibly meant: $[stmt.list] at $6, symbol
hidden

Possible output (a smarter implementation):
interpreter.ypp:56.43-49: reference is invalid: `$stmt.x'
interpreter.ypp:55.30-33:   possibly meant: $then.x at $4
interpreter.ypp:56.52-61: reference is misleading: `$stmt.list'
interpreter.ypp:55.30-33:   possibly meant: $then.list at $4
interpreter.ypp:55.46-54:   possibly meant: $else at $6
without "symbol hidden" messages.
Hmm, It looks worth trying. I will try to implement it. Unfortunately I will
have no time in 2-2.5 weeks.

 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).
>
I always quote the symbol in the main message because it is taken verbatim
from user's input. All other strings in the output are bison "inventions".
I think that is important to show exactly what was written by user -
otherwise user's confidence will decline.


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

 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 attached patch I separated two cases: "symbol not found" when no
variants available, and "reference is invalid" when there is single (but
wrong) variant. So we will have:
zero variants: "symbol not found"
one variant: "reference is invalid" or no errors
two or more variants: "reference is ambiguous" (when no bracketing or hiding
errors present) or "reference is misleading".

>

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

Sorry about that. I very used to write "type* var" and did my best to use
your style in most of the places.


> +  if (('0' <= *cp && *cp <= '9') || *cp == '-')
> I believe we avoid relying on the encoding.  Use isdigit instead.

Actually, this line is coming from existing code. Currently bison does not
use "ctype.h". Should we add this additional dependency ? "ctype.h" is known
to have problems with various versions of libc...


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

I fixed it.


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

I think that it is not a good style to disallow white spaces between '[' and
id. We may discuss it.

I attached a fixed version of the patch.
As for the legal paperwork - the remaining part is purely technical - I've
already submitted the employer disclaimer and awaiting for some paper mail
package.

-- 
Best regards,
Alex Rozenman (address@hidden).

2009-04-04.patch
Description: Text Data