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

Marek Safar marek.safar at seznam.cz
Mon Jul 4 11:48:05 EDT 2005


Hello, 

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

I finally see where the point is ;-)
Unfortunately I have to say our hierarchy structure seems to be broken.
 From my point of view the MemberName should be introduced in DeclSpace 
class and not in MemberCore
where should be only your LocatedToken. But this change is not simple. 
Then we should avoid this for most of cases.

 namespace_or_type_name
 	: member_name
 	| namespace_or_type_name DOT IDENTIFIER {
-		$$ = new MemberName ((MemberName) $1, (string) $3);
+		LocatedToken lt = (LocatedToken) $3;
+		$$ = new MemberName ((MemberName) $1, lt.Value, lt.Location);
 	  }
 	;
 
 member_name
 	: IDENTIFIER {
-		$$ = new MemberName ((string) $1);
+		LocatedToken lt = (LocatedToken) $1;
+		$$ = new MemberName (lt.Value, lt.Location);
 	  }
 	;


Do you have the figures how many times are MemberName ctor and 
LocatedToken ctor called ?


I think this is not correct. `lt.Location' is useless we should always 
print position where identifier begins.

-		$$ = new MemberName ((MemberName) $1, (string) $3);
+		LocatedToken lt = (LocatedToken) $3;
+		$$ = new MemberName ((MemberName) $1, lt.Value, lt.Location)


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

Sorry, I saw MemberBase and not MemberName.

Marek



More information about the Mono-devel-list mailing list