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

[Chandler Carruth]

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?


On Mon, Aug 27, 2012 at 10:51 AM, Richard Trieu <rtrieu at google.com> wrote:

> On Wed, Aug 22, 2012 at 12:59 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Wed, Aug 22, 2012 at 11:25 AM, Richard Trieu <rtrieu at google.com>wrote:
>>
>>> On Tue, Aug 21, 2012 at 7:17 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>>
>>>> On Tue, Aug 21, 2012 at 6:59 PM, Richard Trieu <rtrieu at google.com>wrote:
>>>>
>>>>> Currently, template diffing does not handle cases where the templates
>>>>> have qualifiers.  This can lead to warnings such as:
>>>>>
>>>>> ... 'vector<vector<[...]>>' cannot convert to 'vector<vector<[...]>>'
>>>>> ...
>>>>>
>>>>> when the message means, can't convert from vector<const vector<int>>
>>>>> to vector<vector<int>>.  This patch allows the internal diff tree to store
>>>>> the qualifiers for template types and adds new methods for finding the
>>>>> difference between sets of qualifiers and to print out the qualifiers.
>>>>>  This does not affect non-template types as they already print qualifiers.
>>>>>
>>>>
>>>> Very cool!
>>>>
>>>>
>>>>> Examples:
>>>>> vector<const vector<int>> and vector<vector<int>>
>>>>> inline: vector<const vector<[...]>> vs. vector<(missing const)
>>>>> vector<[...]>
>>>>>
>>>>
>>>> I think I would prefer: vector<const vector<[...]>> vs.
>>>> vector<vector<[...]>>
>>>>
>>>
>>>> We can use the cyan highlighting to further emphasize the qualifiers on
>>>> both sides (if any) when the qualifiers are different. This would make the
>>>> const bright here, and in the example further down, the const bright on one
>>>> side and the volatile on the other.
>>>>
>>>> One reason why I prefer this format is brevity. The other is that the
>>>> error might be the *presence* of const just as easily as the absence.
>>>>
>>>
>>> The motivation behind the (missing ...) was that there's
>>> no guarantee that the types printed would be close to each other.  There
>>> may be cases of : vector<const vector<[...]>> ... long message ...
>>> vector<vector<[...]>>.  Going by your suggestion, the second type would be
>>> completely unhighlighted.
>>>
>>
>> Seems fine to me? As long as the const is highlighted in the first type,
>> I think it's likely to draw the attention to the correct place.
>>
>
> New patch.  This includes the changes Chandler and Richard Smith
> suggested: when printing types inline, don't print (missing ...) for
> missing qualifiers; and maintain consistence by performing qualifier
> highlighting on non-template types used as template arguments.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120827/041ba171/attachment.html>