On Thu, Sep 13, 2012 at 4:44 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Mon, Aug 27, 2012 at 4:29 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div>On Mon, Aug 27, 2012 at 11:04 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


Cool, a couple of high level comments:<div><br></div><div><div>@@ -778,17 +793,19 @@</div><div>           if (Context.hasSameType(FromType, ToType)) {</div><div>             Tree.SetSame(true);</div><div>           } else {</div>



<div>-            const TemplateSpecializationType *FromArgTST =</div><div>-                GetTemplateSpecializationType(Context, FromType);</div><div>-            const TemplateSpecializationType *ToArgTST =</div><div>


-                GetTemplateSpecializationType(Context, ToType);</div>
<div>-</div><div>-            if (FromArgTST && ToArgTST) {</div><div>-              bool SameTemplate = hasSameTemplate(FromArgTST, ToArgTST);</div><div>-              if (SameTemplate) {</div><div>-                Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),</div>



<div>-                             ToArgTST->getTemplateName().getAsTemplateDecl());</div><div>-                DiffTemplate(FromArgTST, ToArgTST);</div><div>+            Qualifiers FromQual, ToQual;</div><div>+            bool SameTemplate = hasSameTemplate(Context, FromType, ToType,</div>



<div>+                                                FromQual, ToQual);</div><div>+            if (SameTemplate) {</div><div>+              const TemplateSpecializationType *FromArgTST =</div><div>+                  GetTemplateSpecializationType(Context, FromType);</div>



<div>+              const TemplateSpecializationType *ToArgTST =</div><div>+                  GetTemplateSpecializationType(Context, ToType);</div><div>+              if (FromArgTST && ToArgTST) {</div><div>+              Tree.SetNode(FromArgTST->getTemplateName().getAsTemplateDecl(),</div>



<div>+                           ToArgTST->getTemplateName().getAsTemplateDecl());</div><div>+              Tree.SetNode(FromQual, ToQual);</div><div>+              DiffTemplate(FromArgTST, ToArgTST);</div><div>               }</div>



<div>             }</div><div>           }</div></div><div><br></div><div>The indent here is now off... and this is crazily nested.</div><div><br></div><div>Can we factor this function into smaller pieces with less nesting? I think that would make it much easier to read.</div>



<div><br></div><div><div>@@ -824,62 +841,120 @@</div><div>     }</div><div>   }</div><div> </div><div>+  static void RemoveQualifiers(Qualifiers &Qual, Qualifiers RemoveQual) {</div></div><div><br></div><div>I feel like this should be a method on Qualifiers?</div>



<div><br></div><div><div>+  // Removes any common qualifiers from ToQual and FromQual and places them</div><div>+  // in CommonQual.</div><div>+  void DiffQualifiers(Qualifiers &FromQual, Qualifiers &ToQual,</div>



</div><div><br></div><div>I feel like this should also be provided by Qualifiers -- probably as a static factory function?</div><div><div><div class="gmail_extra"></div></div></div></blockquote>
<div> </div></div></div><div>Good idea.  I'll make a new patch for the Qualifiers stuff.</div></div>
</blockquote></div><br></div></div><div>Qualifiers functions pulled out and committed in a separate patch.  Updated patch attached.</div>
</blockquote></div><br><div>Another patch.  Fix some issues with handling of templates in typedefs and added a test case for them.</div>