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

[Raphael Isemann via cfe-commits]

teemperor marked an inline comment as done.

================
Comment at: lib/Analysis/CloneDetection.cpp:134
@@ +133,3 @@
+  DEF_ADD_DATA(Stmt, { addData(S->getStmtClass()); })
+  DEF_ADD_DATA(Expr, { addData(S->getType()); })
+
----------------
NoQ wrote:
> 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.
Oh, that's indeed not how it should work! Each parent class should be visited. We could explicitly call the parent class for each visit method but I think having this automated would be good (i.e. like the old patches do it but without the resulting binary size increase). Would appreciate suggestions here :)

================
Comment at: lib/Analysis/CloneDetection.cpp:148
@@ +147,3 @@
+  DEF_ADD_DATA(CallExpr,
+               { addData(S->getDirectCallee()->getQualifiedNameAsString()); })
+
----------------
NoQ wrote:
> 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.
I think we'll save a lot of space with that but it only works for a single TU. I think I'll change it until we actually have multi-TU support.

Yeah, the children types prevent overload collisions (or they should if the visitor wasn't broken).


https://reviews.llvm.org/D22514