[cfe-commits] r127401 - /cfe/trunk/lib/Sema/SemaType.cpp

[John McCall]

On Mar 10, 2011, at 8:41 AM, Abramo Bagnara wrote:

> Il 10/03/2011 17:18, Douglas Gregor ha scritto:
>> 
>> On Mar 10, 2011, at 8:13 AM, Abramo Bagnara wrote:
>> 
>>> Il 10/03/2011 16:39, Douglas Gregor ha scritto:
>>>>> -          // Note: if NNS is dependent, then its prefix (if any) is already
>>>>>> -          // included in ClsType; this does not hold if the NNS is
>>>>>> -          // nondependent: in this case (if there is indeed a prefix)
>>>>>> -          // ClsType needs to be wrapped into an elaborated type.
>>>>>> -          if (NNSPrefix && !NNS->isDependent())
>>>>>> +          // Note: if the NNS has a prefix and ClsType is a nondependent
>>>>>> +          // TemplateSpecializationType, then the NNS prefix is NOT included
>>>>>> +          // in ClsType; hence we wrap ClsType into an ElaboratedType.
>>>>>> +          // NOTE: in particular, no wrap occurs if ClsType already is an
>>>>>> +          // Elaborated, DependentName, or DependentTemplateSpecialization.
>>>>>> +          if (NNSPrefix && isa<TemplateSpecializationType>(NNS->getAsType()))
>>>>>>           ClsType = Context.getElaboratedType(ETK_None, NNSPrefix, ClsType);
>>>>>>         break;
>>>>>>       }
>>>> Do you have a test case for this?
>>> 
>>> Apart some tests in the huge testsuite of our application (not useable
>>> with clang), the output of clang -cc1 -ast-dump showed the omitted NNS.
>>> 
>>> Taken in account that this was a pure syntactical issue, there is a way
>>> to add a testcase for that?
>> 
>> The ideal way to test this would be to test the output of
>> 
>> 	c-index-test -test-annotate-tokens
> 
> I've no idea how to do that.
> 
> I've in past several times rechecked and revalidated the expected output
> for several tests in test/Index (an horrible and tiresome work: the
> representation line:column is very hard to verify), but I've no idea how
> to add other tests.
> 
> To be fully honest, doing that work, several times I stopped myself
> wondering whether that source locations has been only verbatim copied
> (with a prefix) from current output of tools or if they later have
> indeed been checked ;-) I've checked several hundredth of that (the one
> modified by our patches) and I've to say that I didn't enjoyed it.

The output is, in fact, specifically designed to be copied-and-pasted
into a test.  And that's okay!  It's great if you want to do due diligence
and ensure that *all* the source locations are where you want them
to be, but it's acceptable to just check the specific source locations
you're interested in (which should be easy to find by the cursor type).
The way FileCheck works, you can either just leave those specific
lines in (turning the first into a CHECK instead of a CHECK-NEXT),
or you can leave them all there as a general regression test for
changes in reported source locations.

The only problem is that I'm pretty sure they're useless for testing
source locations in an instantiation.

John.