[PATCH] D36664: [analyzer] Make StmtDataCollector customizable

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Very well done, I really like this patch! I added a few remarks mostly about the comments that need some small adjusting.

I'm wondering what would be a nice way of creating a StmtDataCollector that is faster but only works for single translation units (e.g. it only hashes the pointer values of decls instead of their qualified name)? The use case here would be Stmt::Profile and the CloneDetector which could be set to a single-TU-mode in the CloneChecker. For Stmt::Profile it would ensure we don't degrade performance from the current version, for the CloneChecker we probably get a reasonable performance boost (as the hashing is currently the last remaining bottle neck).

@arphaman Any suggestions who could review/approve the additions to `AST/`?



================
Comment at: include/clang/AST/DataCollection.h:19
+
+namespace clang {
+
----------------
I think we need another namespace here because I don't like a generic function name like `addDataToConsumer` in the `clang` namespace. Maybe `clang::data_collection::addDataToConsumer` or something like that.


================
Comment at: lib/AST/StmtDataCollectors.inc:5
+  addData(S->getStmtClass());
+  // This ensures that macro generated code isn't identical to macro-generated
+  // code.
----------------
I know this is from my code, but as you're anyway touching this line, could you fix this comment? It should be "that non-macro-generated code` instead of `that macro generated`. Thanks!


================
Comment at: lib/Analysis/CloneDetection.cpp:172
 
+/// Collects the data of a single Stmt.
+///
----------------
I think this line can be removed now.


================
Comment at: lib/Analysis/CloneDetection.cpp:174
+///
+/// This class defines what a code clone is: If it collects for two statements
+/// the same data, then those two statements are considered to be clones of each
----------------
`what a code clone is ` -> `what a type II code clone is`


================
Comment at: lib/Analysis/CloneDetection.cpp:210
+
+// Override the definition for some visitors.
+#define SKIP(CLASS)                                                            \
----------------
Should be `Type II clones ignore variable names and literals, so let's skip them`


https://reviews.llvm.org/D36664