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

[Richard Trieu]

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/224a9c73/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qualifers-type-diffing4.patch
Type: application/octet-stream
Size: 21349 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120928/224a9c73/attachment.obj>