[analyzer] CloneDetector now checks template arguments of function calls.

[teemperor]

Summary

The CloneDetector currently ignores template arguments in function calls which leads to false-positive clones such as isa<Expr>(S) and isa<Stmt>(S).

This patch adds functionality to handle template arguments for those function calls.

[NoQ]

lib/Analysis/CloneDetection.cpp

154

Maybe if (const FunctionDecl *D = S->getDirectCallee()) here, and then use D everywhere inside the if?

164

We don't discriminate between foo<A, AA>() and foo<AA, A>() here, because in both cases the stuff we hash gets concatenated to AAA. Just thought that we might be having this common problem everywhere, even though it barely shows up "in real life".

test/Analysis/copypaste/call.cpp

41

"temp" feels a lot more like "temporary" than like "template" (?)

[v.g.vassilev]

LGTM once the @NoQ remarks are addressed.

[teemperor]

lib/Analysis/CloneDetection.cpp

164

Yes, that could be a problem. I'll fix this issue here in this patch and the others in another patch.

lib/Analysis/CloneDetection.cpp

public:
   void addData(CloneDetector::DataPiece Integer) {
     DataConsumer.update(
         StringRef(reinterpret_cast<char *>(&Integer), sizeof(Integer)));
   }
 
   void addData(llvm::StringRef Str) { DataConsumer.update(Str); }
 
   void addData(const QualType &QT) { addData(QT.getAsString()); }
 
 // The functions below collect the class specific data of each Stmt subclass.
 
 // Utility macro for defining a visit method for a given class. This method
 // calls back to the ConstStmtVisitor to visit all parent classes.
 #define DEF_ADD_DATA(CLASS, CODE)                                              \
   void Visit##CLASS(const CLASS *S) {                                          \
     CODE;                                                                      \
     ConstStmtVisitor<StmtDataCollector>::Visit##CLASS(S);                      \
   }
 
   DEF_ADD_DATA(Stmt, {
     addData(S->getStmtClass());
     // This ensures that macro generated code isn't identical to macro-generated
     // code.
     addData(getMacroStack(S->getLocStart(), Context));
     addData(getMacroStack(S->getLocEnd(), Context));
   })
   DEF_ADD_DATA(Expr, { addData(S->getType()); })
 
   //--- Builtin functionality ----------------------------------------------//
   DEF_ADD_DATA(ArrayTypeTraitExpr, { addData(S->getTrait()); })
   DEF_ADD_DATA(ExpressionTraitExpr, { addData(S->getTrait()); })
   DEF_ADD_DATA(PredefinedExpr, { addData(S->getIdentType()); })
   DEF_ADD_DATA(TypeTraitExpr, {
     addData(S->getTrait());
     for (unsigned i = 0; i < S->getNumArgs(); ++i)
       addData(S->getArg(i)->getType());
   })
 
   //--- Calls --------------------------------------------------------------//
   DEF_ADD_DATA(CallExpr, {
     // Function pointers don't have a callee and we just skip hashing it.
-    if (S->getDirectCallee())
-      addData(S->getDirectCallee()->getQualifiedNameAsString());
+    if (const FunctionDecl *D = S->getDirectCallee()) {
+      // If the function is a template instantiation, we also need to handle
+      // the template arguments as they are no included in the qualified name.
+      if (D->isTemplateInstantiation()) {
+        auto Args = D->getTemplateSpecializationArgs();
+        std::string ArgString;
+
+        // Print all template arguments into ArgString
+        llvm::raw_string_ostream OS(ArgString);
+        for (unsigned i = 0; i < Args->size(); ++i) {
+          Args->get(i).print(Context.getLangOpts(), OS);
+          // Add a padding character so that 'foo<X, XX>()' != 'foo<XX, X>()'.
+          OS << '\n';
+        }
+        OS.flush();
+
+        addData(ArgString);
+      }
+      addData(D->getQualifiedNameAsString());
+    }
   })
 
   //--- Exceptions ---------------------------------------------------------//
   DEF_ADD_DATA(CXXCatchStmt, { addData(S->getCaughtType()); })
 
   //--- C++ OOP Stmts ------------------------------------------------------//
   DEF_ADD_DATA(CXXDeleteExpr, {
     addData(S->isArrayFormAsWritten());

test/Analysis/copypaste/call.cpp

 
 bool fooPtr1(int x) {
   if (x > 0)
     return false;
   else if (x < 0)
     return funcPtr(1);
   return true;
 }
+
+// Test that we respect the template arguments of function templates
+
+template<typename T, unsigned N>
+bool templateFunc() { unsigned i = N; return false; }
+
+bool fooTemplate1(int x) {
+  if (x > 0)
+    return false;
+  else if (x < 0)
+    return templateFunc<int, 1>();
+  return true;
+}
+
+bool fooTemplate2(int x) {
+  if (x > 0)
+    return false;
+  else if (x < 0)
+    return templateFunc<long, 1>();
+  return true;
+}
+
+bool fooTemplate3(int x) {
+  if (x > 0)
+    return false;
+  else if (x < 0)
+    return templateFunc<long, 2>();
+  return true;
+}
+
+// Test that we don't just concatenate the template arguments into a string
+// without having any padding between them (e.g. foo<X, XX>() != foo<XX, X>()).
+
+class X {};
+class XX {};
+
+template<typename T1, typename T2>
+bool templatePaddingFunc() { return false; }
+
+bool fooTemplatePadding1(int x) {
+  if (x > 0)
+    return false;
+  else if (x < 0)
+    return templatePaddingFunc<X, XX>();
+  return true;
+}
+
+bool fooTemplatePadding2(int x) {
+  if (x > 0)
+    return false;
+  else if (x < 0)
+    return templatePaddingFunc<XX, X>();
+  return true;
+}