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

[Douglas Gregor]

On Mar 10, 2011, at 9:43 AM, John McCall wrote:

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

Right. We want to know when our source-location information has *changed*. If it's gotten better, great! We fix the output. If it's gotten worse, we know that we've regressed. But we won't know if it's changed if we have no tests.

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

Yeah, the only way we have for testing source locations in an instantiation is to cause a failed instantiation and stretch out the text so that we can do line-specific checks. That's why the source-location tests have weird bits like

	typedef X<
		typename add_reference<T>::type
			* // expected-error{{verify that clang points here when complaining about the pointer instantiation}}
		> type;

	- Doug