[PATCH] D22514: CloneDetection now respects statement specific data when searching for clones.

[Artem Dergachev via cfe-commits]

NoQ added inline comments.

================
Comment at: lib/Analysis/CloneDetection.cpp:91
@@ +90,3 @@
+  ASTContext &Context;
+  std::vector<unsigned> &CollectedData;
+
----------------
Maybe it's a good idea to introduce a typedef for `unsigned` here, because it'd be nice to be able to change it (eg., some day we may desire to switch to uint64_t).

================
Comment at: lib/Analysis/CloneDetection.cpp:119
@@ +118,3 @@
+    // with zeroes.
+    if (Str.size() % sizeof(unsigned))
+      CollectedData.push_back(0);
----------------
Maybe just allocate the right amount of bytes during resize? Some usual trick with rounding up, eg. `(x + y - 1) / y`.

================
Comment at: lib/Analysis/CloneDetection.cpp:134
@@ +133,3 @@
+  DEF_ADD_DATA(Stmt, { addData(S->getStmtClass()); })
+  DEF_ADD_DATA(Expr, { addData(S->getType()); })
+
----------------
I noticed something: with this patch, you no longer consider the kind of the statement for supported statement types (even for, say, `Expr`), because the visitor doesn't automatically fall back to parent classes when a method for the child class is defined. Is this intentional? Or you may want to visit some statements with parent methods manually.

================
Comment at: lib/Analysis/CloneDetection.cpp:148
@@ +147,3 @@
+  DEF_ADD_DATA(CallExpr,
+               { addData(S->getDirectCallee()->getQualifiedNameAsString()); })
+
----------------
I wonder how much we can save if we use a few pointers-based hashes instead of hashing long fully-qualified names, for stuff like function decls or types.

Uhm, by the way, in fact C++ overloaded functions have same qualified names, do we consider that? I guess sub-statement hashing would most likely take care of that.

================
Comment at: test/Analysis/copypaste/test-asm.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -std=c++11 -analyzer-checker=alpha.clone.CloneChecker -verify %s
+
----------------
Do we really need to prefix all test file names with "`test-`"? It's kind of obvious :)


https://reviews.llvm.org/D22514