SourceRange for QualifiedTypeLoc

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

SourceRange for QualifiedTypeLoc

Abramo Bagnara-2

In QualifiedTypeLoc currently we have:

  SourceRange getSourceRange() const {
    return SourceRange();
  }

Taken for granted missing location of qualifiers I think that it would
be a bit better to have:

  SourceRange getSourceRange() const {
    return getUnqualifiedLoc().getSourceRange();
  }

Can I proceed in this direction?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

John McCall

On May 19, 2010, at 4:00 AM, Abramo Bagnara wrote:

>
> In QualifiedTypeLoc currently we have:
>
>  SourceRange getSourceRange() const {
>    return SourceRange();
>  }
>
> Taken for granted missing location of qualifiers I think that it would
> be a bit better to have:
>
>  SourceRange getSourceRange() const {
>    return getUnqualifiedLoc().getSourceRange();
>  }
>
> Can I proceed in this direction?

Generally a TypeLoc's getSourceRange() returns the source range
corresponding to that portion of the type, i.e. the "local" source range.
Since we don't record the qualifier locations, a QualifiedTypeLoc's source
range should be empty.  Clients should use getFullSourceRange() if they
want the source range for the entire type.

That said, I'm willing to consider deviating from that design if it's causing
problems.

...also, the names aren't great;  I wouldn't object to a couple of renames:
  getSourceRange -> getLocalSourceRange
  getFullSourceRange -> getSourceRange

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

Abramo Bagnara-2
Il 19/05/2010 21:43, John McCall ha scritto:
>
> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>   getSourceRange -> getLocalSourceRange
>   getFullSourceRange -> getSourceRange

Done in r104220.

I'll follow with a commit that fixes typeloc getSourceRange for
QualifiedTypeLoc, ElaboratedTypeLoc and DependentNameTypeLoc.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

Abramo Bagnara-2
Il 20/05/2010 12:11, Abramo Bagnara ha scritto:
> Il 19/05/2010 21:43, John McCall ha scritto:
>>
>> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>>   getSourceRange -> getLocalSourceRange
>>   getFullSourceRange -> getSourceRange
>
> Done in r104220.

I'm under the impression that *every* use of getLocalSourceRange outside
TypeLoc.hh and TypeLoc.cpp is bogus...

My proposal is:

- add TypeLoc::getBeginLoc() and TypeLoc::getEndLoc() with a possibily
specialized implementation (at least for Elaborated and DependendentName
and QualifiedTypeLoc)

- implement getSourceRange() using the two just added method

- transform getLocalSourceRange().getBegin() in getBeginLoc()

- transform getLocalSourceRange().getEnd() in getEndLoc()

- transform getLocalSourceRange() in getSourceRange()

- move getLocalSourceRange() to protected area.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

John McCall

On May 20, 2010, at 6:05 AM, Abramo Bagnara wrote:

> Il 20/05/2010 12:11, Abramo Bagnara ha scritto:
>> Il 19/05/2010 21:43, John McCall ha scritto:
>>>
>>> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>>>  getSourceRange -> getLocalSourceRange
>>>  getFullSourceRange -> getSourceRange
>>
>> Done in r104220.

Thanks.

> I'm under the impression that *every* use of getLocalSourceRange outside
> TypeLoc.hh and TypeLoc.cpp is bogus...

No;  the index support uses it legitimately in order to narrow down where the
query location is.

Feel free to rework that code if it makes your task easier;  just keep in mind that
its performance is quite important.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

John McCall
In reply to this post by Abramo Bagnara-2

On May 20, 2010, at 6:05 AM, Abramo Bagnara wrote:

> Il 20/05/2010 12:11, Abramo Bagnara ha scritto:
>> Il 19/05/2010 21:43, John McCall ha scritto:
>>>
>>> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>>> getSourceRange -> getLocalSourceRange
>>> getFullSourceRange -> getSourceRange
>>
>> Done in r104220.

Thanks.

> I'm under the impression that *every* use of getLocalSourceRange outside
> TypeLoc.hh and TypeLoc.cpp is bogus...

No;  the index support uses it legitimately in order to narrow down where the
query location is.

Feel free to rework that code if it makes your task easier;  just keep in mind that
its performance is quite important.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

Abramo Bagnara-2
Il 20/05/2010 19:10, John McCall ha scritto:

>
> On May 20, 2010, at 6:05 AM, Abramo Bagnara wrote:
>
>> Il 20/05/2010 12:11, Abramo Bagnara ha scritto:
>>> Il 19/05/2010 21:43, John McCall ha scritto:
>>>>
>>>> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>>>> getSourceRange -> getLocalSourceRange
>>>> getFullSourceRange -> getSourceRange
>>>
>>> Done in r104220.
>
> Thanks.
>
>> I'm under the impression that *every* use of getLocalSourceRange outside
>> TypeLoc.hh and TypeLoc.cpp is bogus...
>
> No;  the index support uses it legitimately in order to narrow down where the
> query location is.
>
> Feel free to rework that code if it makes your task easier;  just keep in mind that
> its performance is quite important.
I've attached the patch to fix typeloc sourcerange for review/approval.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

TypeLocRange.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

John McCall

On May 21, 2010, at 3:33 AM, Abramo Bagnara wrote:

> Il 20/05/2010 19:10, John McCall ha scritto:
>>
>> On May 20, 2010, at 6:05 AM, Abramo Bagnara wrote:
>>
>>> Il 20/05/2010 12:11, Abramo Bagnara ha scritto:
>>>> Il 19/05/2010 21:43, John McCall ha scritto:
>>>>>
>>>>> ...also, the names aren't great;  I wouldn't object to a couple of renames:
>>>>> getSourceRange -> getLocalSourceRange
>>>>> getFullSourceRange -> getSourceRange
>>>>
>>>> Done in r104220.
>>
>> Thanks.
>>
>>> I'm under the impression that *every* use of getLocalSourceRange outside
>>> TypeLoc.hh and TypeLoc.cpp is bogus...
>>
>> No;  the index support uses it legitimately in order to narrow down where the
>> query location is.
>>
>> Feel free to rework that code if it makes your task easier;  just keep in mind that
>> its performance is quite important.
>
> I've attached the patch to fix typeloc sourcerange for review/approval.
> <TypeLocRange.patch>

1.  These are really too large to be in TypeLoc.h;  please move them to
TypeLoc.cpp.

2.  DependentNameTypeLoc does not have an inner type and therefore does not
need this logic.

As a side note, even if we added source locations to QualifiedTypeLoc they would
not be reliably ordered w.r.t. the inner type;  in fact, in most places they come *after*
the inner type, not before.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceRange for QualifiedTypeLoc

Abramo Bagnara-2
Il 21/05/2010 19:29, John McCall ha scritto:
>
>> I've attached the patch to fix typeloc sourcerange for review/approval.
>> <TypeLocRange.patch>
>
> 1.  These are really too large to be in TypeLoc.h;  please move them to
> TypeLoc.cpp.
>
> 2.  DependentNameTypeLoc does not have an inner type and therefore does not
> need this logic.

Commited with your suggestions in r104382.

> As a side note, even if we added source locations to QualifiedTypeLoc they would
> not be reliably ordered w.r.t. the inner type;  in fact, in most places they come *after*
> the inner type, not before.

Also inside, like in: unsigned const long volatile long a;

Nevertheless I guess it might be possible to return an exact source
range for a QualifiedTypeLoc using something like min(begin1, begin2),
max(end1, end2) where min and max are implemented using
SourceManager::isBeforeInTranslationUnit.


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev