LGTM On Fri, Sep 28, 2012 at 1:21 PM, Richard Trieu <[email protected]> wrote:
> On Wed, Sep 26, 2012 at 7:22 PM, Richard Smith <[email protected]>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 <[email protected]> wrote: >> >>> On Mon, Aug 27, 2012 at 4:29 PM, Richard Trieu <[email protected]>wrote: >>> >>>> On Mon, Aug 27, 2012 at 11:04 AM, Chandler Carruth < >>>> [email protected]> 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. >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
