[Mono-devel-list] mcs patch for precise location handling

[Atsushi Eno]

Hi,

Martin gave some comments on the patch, so I made a few changes:

	- removed "dont_progress_location" conditions in tokenizer
	- fixed incorrectly modified "public int token" in location.cs
	- renamed "StringValue" to "Value" in LocatedToken.

And merged the patch to the latest trunk as:
http://monkey.workarea.jp/tmp/20050704/precise-location-20050704.diff

> Only where it is appropriate not for `if', `foreach', etc.
> I think it will be easier to read and probably a little bit faster.
> 
> Like this
> 
> +        LocatedToken lt = (LocatedToken) $1;
> +        LabeledStatement labeled = new LabeledStatement 
> (lt.StringValue, lt.Location);
> 
> or this
> 
> -        CheckBinaryOperator ((Operator.OpType) $3);
> +        LocatedToken ltParam1 = (LocatedToken) $6;
> +        LocatedToken ltParam2 = (LocatedToken) $9;
> +        CheckBinaryOperator ((Operator.OpType) $3, (Location) $2);
> 
>         Parameter [] pars = new Parameter [2];
> 
>         Expression typeL = (Expression) $5;
>         Expression typeR = (Expression) $8;
> 
> -           pars [0] = new Parameter (typeL, (string) $6, 
> Parameter.Modifier.NONE, null, lexer.Location);
> -           pars [1] = new Parameter (typeR, (string) $9, 
> Parameter.Modifier.NONE, null, lexer.Location);
> +           pars [0] = new Parameter (typeL, ltParam1.StringValue, 
> Parameter.Modifier.NONE, null, ltParam1.Location);
> +           pars [1] = new Parameter (typeR, ltParam2.StringValue, 
> Parameter.Modifier.NONE, null, ltParam2.Location);
> 
>>
>> I once thought about that, but it makes those constructors too
>> parser-specific while some of them are invoked inside some other
>> types (Iterators etc.) where we have dare
>> to create LocatedToken. So I felt that it won't be worthy of
>> making those changes just to make parser a bit readable. Especially
>> I was afraid that there might be need for LocatedToken-less
>> ctor()s in the future (or that could already happen in gmcs).
> 
> I think 99% are parser specific. Otherwise Iterator can always create 
> this container.
> I cannot talk about gmcs as I don't know the sources.

Am so skeptical about the good effect (code readability) as
compared to the bad effect (performance, cost of work, and in
fact code readability also here). Sure, it might improve
readability for some readers, but on the other hand it introduces
extraneous new LocatedToken() in its internal processing stage.

Actally I tried to make such changes from reviewing all LocatedToken
usage in cs-parser.jay, but there were much more cases that I had
to change. For example when I changed LocalVariableReference.ctor()
to accept LocatedToken, it requires to introduce "new LocatedToken()"
in DoSimpleNameResolve() whose invocation count is not small in
the profiler result.

(Of course avoiding LocatedToken for such specific cases will
just make code inconsistent.)

The changes are also being so massive that I wonder if it is
really worthy of changing. It is far from mandatory changes.

If the majority of mcs hackers still want this change, I'll
restart this.

> But you don't need to introduce the `LocatedToken' when you just reuse 
> `MemberName'.
> It will simplify this mainly when you rewrite `VariableDeclaration' to 
> hold this MemberName then
> we will have just 1 name container.

Yes, but I am not sure if member names and variable name are in
the same semantics (esp. am not sure if those constraints are/
will be the same) or not and thus if MemberName can be used or not.

>>> MemberBase is derived from MemberCore
>>>
>>> +Location GetLocation (object obj)
>>> +{
>>> +    if (obj is MemberCore)
>>> +        return ((MemberCore) obj).Location;
>>> +    if (obj is MemberName)
>>> +        return ((MemberName) obj).Location;
>>
>>
>> Mmm, I couldn't understand what you meant. MemberBase will be
>> thus handled here, so I don't see possible improvements here.
> 
> +    if (obj is MemberName)
> This `if' never happens.

Well, I don't think MemberName is derived from MemberCore.

Atsushi Eno