Re: [cfe-commits] [Patch] - Avoid duplicated visits to default arguments for class templates

[Yang Chen]

Hi Richard,

On 5/29/12 6:41 PM, Richard Smith wrote:
Hi,
On Sun, May 27, 2012 at 6:29 PM, Yang Chen <cheny...@cs.utah.edu 
<mailto:cheny...@cs.utah.edu>> wrote:

    Seems RecursiveASTVisitor has a bug which could cause duplicated
    visits to default arguments for a class template. For example,
    given the code below:

    class X;
    template<class T = X> class Y;
    template<class T> class Y {};

    The default argument 'X' will be visited twice. The attached patch
    fixed the problem.


Thanks, the patch looks good. We should also handle 
NonTypeTemplateParmDecl in the same way.

Fixed.

    I also attached a small testcase. Thanks.


(It's conventional to put the code and test changes in the same patch 
file.) Tests for the other two flavours of template parameters would 
be great (you will probably need to add new ExpectedLocationVisitor 
subclasses to RecursiveASTVisitorTest to test those).

Thanks for the info. I attached a new patch which includes two more test 
cases for TemplateTemplateParmDecl and NonTypeTemplateParmDecl.

- Yang

Index: include/clang/AST/RecursiveASTVisitor.h
===================================================================
--- include/clang/AST/RecursiveASTVisitor.h     (revision 157680)
+++ include/clang/AST/RecursiveASTVisitor.h     (working copy)
@@ -1486,7 +1486,7 @@
     // D is the "T" in something like
     //   template <template <typename> class T> class container { };
     TRY_TO(TraverseDecl(D->getTemplatedDecl()));
-    if (D->hasDefaultArgument()) {
+    if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited()) {
       TRY_TO(TraverseTemplateArgumentLoc(D->getDefaultArgument()));
     }
     TRY_TO(TraverseTemplateParameterListHelper(D->getTemplateParameters()));
@@ -1496,7 +1496,7 @@
     // D is the "T" in something like "template<typename T> class vector;"
     if (D->getTypeForDecl())
       TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
-    if (D->hasDefaultArgument())
+    if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited())
       TRY_TO(TraverseTypeLoc(D->getDefaultArgumentInfo()->getTypeLoc()));
   })
 
@@ -1766,7 +1766,8 @@
 DEF_TRAVERSE_DECL(NonTypeTemplateParmDecl, {
     // A non-type template parameter, e.g. "S" in template<int S> class Foo ...
     TRY_TO(TraverseDeclaratorHelper(D));
-    TRY_TO(TraverseStmt(D->getDefaultArgument()));
+    if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited())
+      TRY_TO(TraverseStmt(D->getDefaultArgument()));
   })
 
 DEF_TRAVERSE_DECL(ParmVarDecl, {
Index: unittests/Tooling/RecursiveASTVisitorTest.cpp
===================================================================
--- unittests/Tooling/RecursiveASTVisitorTest.cpp       (revision 157680)
+++ unittests/Tooling/RecursiveASTVisitorTest.cpp       (working copy)
@@ -185,6 +185,33 @@
   }
 };
 
+class TemplateArgumentLocTraverser
+  : public ExpectedLocationVisitor<TemplateArgumentLocTraverser> {
+public:
+  bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &ArgLoc) {
+    std::string ArgStr;
+    llvm::raw_string_ostream Stream(ArgStr);
+    const TemplateArgument &Arg = ArgLoc.getArgument();
+
+    Arg.print(Context->getPrintingPolicy(), Stream);
+    Match(Stream.str(), ArgLoc.getLocation());
+    return ExpectedLocationVisitor<TemplateArgumentLocTraverser>::
+      TraverseTemplateArgumentLoc(ArgLoc);
+  }
+};
+
+class CXXBoolLiteralExprVisitor 
+  : public ExpectedLocationVisitor<CXXBoolLiteralExprVisitor> {
+public:
+  bool VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *BE) {
+    if (BE->getValue())
+      Match("true", BE->getLocation());
+    else
+      Match("false", BE->getLocation());
+    return true;
+  }
+};
+
 TEST(RecursiveASTVisitor, VisitsBaseClassDeclarations) {
   TypeLocVisitor Visitor;
   Visitor.ExpectMatch("class X", 1, 30);
@@ -394,4 +421,31 @@
   EXPECT_TRUE(Visitor.runOver("int k = (4) + 9;\n"));
 }
 
+TEST(RecursiveASTVisitor, VisitsClassTemplateNonTypeParmDefaultArgument) {
+  CXXBoolLiteralExprVisitor Visitor;
+  Visitor.ExpectMatch("true", 2, 19);
+  EXPECT_TRUE(Visitor.runOver(
+    "template<bool B> class X;\n"
+    "template<bool B = true> class Y;\n"
+    "template<bool B> class Y {};\n"));
+}
+
+TEST(RecursiveASTVisitor, VisitsClassTemplateTypeParmDefaultArgument) {
+  TypeLocVisitor Visitor;
+  Visitor.ExpectMatch("class X", 2, 23);
+  EXPECT_TRUE(Visitor.runOver(
+    "class X;\n"
+    "template<typename T = X> class Y;\n"
+    "template<typename T> class Y {};\n"));
+}
+
+TEST(RecursiveASTVisitor, VisitsClassTemplateTemplateParmDefaultArgument) {
+  TemplateArgumentLocTraverser Visitor;
+  Visitor.ExpectMatch("X", 2, 40);
+  EXPECT_TRUE(Visitor.runOver(
+    "template<typename T> class X;\n"
+    "template<template <typename> class T = X> class Y;\n"
+    "template<template <typename> class T> class Y {};\n"));
+}
+
 } // end namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits