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

[Atsushi Eno]

Hi Marek,

Thanks, as usual. I put a revised patch here:
http://monkey.workarea.jp/tmp/20050630/precise-location-20050630.diff


> I think can be useful make 'public class LocatedToken' struct as is 
> passed only as an argument.
> And use this class in similar way as MemberName for every relevant 
> consumers.

Do you mean, passing LocatedToken as an argument for each MemberCore-
derived types, such as Class, Struct, Event etc.?

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

For VariableDeclaration, I made the change because it seems for
parser internal use (it is defined in cs-parser.jay). For
IndexerDeclaration I didn't change because setting location to
"this" would rather make sense.

> For example:
> 
> @@ -334,17 +334,18 @@
>     : USING IDENTIFIER ASSIGN       namespace_or_type_name SEMICOLON
>       {
> -        current_namespace.UsingAlias ((string) $2, (MemberName) $4, 
> lexer.Location);
> +        LocatedToken lt = (LocatedToken) $2;
> +        current_namespace.UsingAlias (lt.StringValue, (MemberName) $4, 
> lt.Location);
>       }
>     | USING error {
> -        CheckIdentifierToken (yyToken);
> +        CheckIdentifierToken (yyToken, GetLocation ($2));
>       }
>     ;
> using_namespace_directive
>     : USING namespace_name SEMICOLON       {
> -        current_namespace.Using ((MemberName) $2, lexer.Location);
> +        current_namespace.Using ((MemberName) $2, (Location) $1);
>           }
>     ;

Well, strictly to say, UsingAlias is located at the beginning of
the word "using", so the right Location should be (Location) $1.
I changed in in the new patch.

BTW the last patch had a tiny bug that cs1537.cs reports (0,1) when
I changed the line above to use $1 while it should report (4,1).
That is a bug in location.cs that tokens on the first line has
incorrect line number (0).

> next example can be this one.
> 
> constant_declarator
>     : IDENTIFIER ASSIGN constant_expression
>       {
> -        $$ = new VariableDeclaration ((string) $1, $3, lexer.Location);
> +        LocatedToken lt = (LocatedToken) $1;
> +        $$ = new VariableDeclaration (lt.StringValue, $3, lt.Location);
>       }

It depends on the premise that I had better pass LocatedToken to
.ctor().

> One more MemberName
> 
>         current_namespace = new NamespaceEntry (
> -            current_namespace, file, name.GetName (), lexer.Location);
> +            current_namespace, file, name.GetName (), name.Location);
>       }

ditto.

> Why is there so many locations ?
> 
> @@ -475,7 +475,7 @@
> 
>         void Define_Fields ()
>         {
> -            Location loc = Location.Null;
> +            Location loc = Location;
> 
>             pc_field = new Field (
>                 this, TypeManager.system_int32_expr, Modifiers.PRIVATE, 
> "PC",

Field.ctor() still requires Location since it is not passed MemberName
(MemberName is created in the .ctor() itself).

> @@ -536,8 +536,7 @@
>             Constructor ctor = new Constructor (
>                 this, Name, Modifiers.PUBLIC, ctor_params,
>                 new ConstructorBaseInitializer (
> -                    null, Parameters.EmptyReadOnlyParameters, Location),
> -                Location);
> +                    null, Parameters.EmptyReadOnlyParameters, 
> Location), Location);
>             AddConstructor (ctor);

The first Location is for ConstructorInitializer and the second one
is for Constructor. cs-parser.jay tells that constructor_initializer
is optional, so it might be null. Thus I cannot omit the Location
argument in Constructor.ctor().

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

> The instance property IsNull can be more suitable.
> 
> -                if (location.Equals (Location.Null)) {
> +                if (Location.IsNull (location)) {

Yeah, sounds good. It is done.

Atsushi Eno