[cfe-commits] [Patch] Handle qualifiers for template types in during template type diffing.

[Richard Smith]

LGTM

On Fri, Sep 28, 2012 at 1:21 PM, Richard Trieu <rtrieu at google.com> wrote:

> On Wed, Sep 26, 2012 at 7:22 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> Hi Richard!
>>
>> @@ -778,17 +793,19 @@
>>            if (Context.hasSameType(FromType, ToType)) {
>>              Tree.SetSame(true);
>>            } else {
>> -            const TemplateSpecializationType *FromArgTST =
>> -                GetTemplateSpecializationType(Context, FromType);
>> -            const TemplateSpecializationType *ToArgTST =
>> -                GetTemplateSpecializationType(Context, ToType);
>>  -
>> -            if (FromArgTST && ToArgTST) {
>> -              bool SameTemplate = hasSameTemplate(FromArgTST, ToArgTST);
>> -              if (SameTemplate) {
>> -
>>  Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),
>> -
>> ToArgTST->getTemplateName().getAsTemplateDecl());
>> -                DiffTemplate(FromArgTST, ToArgTST);
>> +            Qualifiers FromQual, ToQual;
>> +            bool SameTemplate = hasSameTemplate(Context, FromType,
>> ToType,
>> +                                                FromQual, ToQual);
>> +            if (SameTemplate) {
>> +              const TemplateSpecializationType *FromArgTST =
>> +                  GetTemplateSpecializationType(Context, FromType);
>> +              const TemplateSpecializationType *ToArgTST =
>> +                  GetTemplateSpecializationType(Context, ToType);
>> +              if (FromArgTST && ToArgTST) {
>> +
>>  Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),
>> +
>> ToArgTST->getTemplateName().getAsTemplateDecl());
>> +              Tree.SetNode(FromQual, ToQual);
>> +              DiffTemplate(FromArgTST, ToArgTST);
>>
>> Missing some indentation here.
>>
>> +    FromType = FromIter[-1].second;
>> +    ToType = ToIter[-1].second;
>> +
>> +    // Remove all the qualifiers that are already in the type.  This gets
>> +    // all the qualifiers that are added between this qualifier and the
>> +    // original type passed into this function.
>> +    FromQual -= FromType.getLocalUnqualifiedType().getQualifiers();
>>
>> It looks like that's the same as QualType(FromIter[-1].first,
>> 0).getQualifiers(). Using that would avoid the need to track QualTypes as
>> you descend through alias templates. Maybe also move this computation out
>> into the caller (where it's "QualType(FromArgTST, 0).getQualifiers()")?
>>
>> Thanks!
>>
>> On Thu, Sep 13, 2012 at 4:44 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>>> On Mon, Aug 27, 2012 at 4:29 PM, Richard Trieu <rtrieu at google.com>wrote:
>>>
>>>> On Mon, Aug 27, 2012 at 11:04 AM, Chandler Carruth <
>>>> chandlerc at google.com> wrote:
>>>>
>>>>> Cool, a couple of high level comments:
>>>>>
>>>>> @@ -778,17 +793,19 @@
>>>>>            if (Context.hasSameType(FromType, ToType)) {
>>>>>              Tree.SetSame(true);
>>>>>            } else {
>>>>> -            const TemplateSpecializationType *FromArgTST =
>>>>> -                GetTemplateSpecializationType(Context, FromType);
>>>>> -            const TemplateSpecializationType *ToArgTST =
>>>>> -                GetTemplateSpecializationType(Context, ToType);
>>>>> -
>>>>> -            if (FromArgTST && ToArgTST) {
>>>>> -              bool SameTemplate = hasSameTemplate(FromArgTST,
>>>>> ToArgTST);
>>>>> -              if (SameTemplate) {
>>>>> -
>>>>>  Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),
>>>>> -
>>>>> ToArgTST->getTemplateName().getAsTemplateDecl());
>>>>> -                DiffTemplate(FromArgTST, ToArgTST);
>>>>> +            Qualifiers FromQual, ToQual;
>>>>> +            bool SameTemplate = hasSameTemplate(Context, FromType,
>>>>> ToType,
>>>>> +                                                FromQual, ToQual);
>>>>> +            if (SameTemplate) {
>>>>> +              const TemplateSpecializationType *FromArgTST =
>>>>> +                  GetTemplateSpecializationType(Context, FromType);
>>>>> +              const TemplateSpecializationType *ToArgTST =
>>>>> +                  GetTemplateSpecializationType(Context, ToType);
>>>>> +              if (FromArgTST && ToArgTST) {
>>>>> +
>>>>>  Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),
>>>>> +
>>>>> ToArgTST->getTemplateName().getAsTemplateDecl());
>>>>> +              Tree.SetNode(FromQual, ToQual);
>>>>> +              DiffTemplate(FromArgTST, ToArgTST);
>>>>>                }
>>>>>              }
>>>>>            }
>>>>>
>>>>> The indent here is now off... and this is crazily nested.
>>>>>
>>>>> Can we factor this function into smaller pieces with less nesting? I
>>>>> think that would make it much easier to read.
>>>>>
>>>>> @@ -824,62 +841,120 @@
>>>>>      }
>>>>>    }
>>>>>
>>>>> +  static void RemoveQualifiers(Qualifiers &Qual, Qualifiers
>>>>> RemoveQual) {
>>>>>
>>>>> I feel like this should be a method on Qualifiers?
>>>>>
>>>>> +  // Removes any common qualifiers from ToQual and FromQual and
>>>>> places them
>>>>> +  // in CommonQual.
>>>>> +  void DiffQualifiers(Qualifiers &FromQual, Qualifiers &ToQual,
>>>>>
>>>>> I feel like this should also be provided by Qualifiers -- probably as
>>>>> a static factory function?
>>>>>
>>>>
>>>> Good idea.  I'll make a new patch for the Qualifiers stuff.
>>>>
>>>
>>> Qualifiers functions pulled out and committed in a separate patch.
>>>  Updated patch attached.
>>>
>>
>>
> New patch, with the much simpler Qualifers handling.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120928/74debffa/attachment.html>