[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think this is awesome.

Would anybody like to move concrete constraint class definitions to a separate header + translation unit, so that to highlight how simple the primary interface is to a person who haven't gone through all the discussion?



================
Comment at: include/clang/Analysis/CloneDetection.h:172
+  /// Generates and stores search data for all statements in the body of
+  ///        the given Decl.
+  void analyzeCodeBody(const Decl *D);
----------------
Strange spacing.


================
Comment at: include/clang/Analysis/CloneDetection.h:178
+  template <typename T>
+  void constrainClones(std::vector<CloneGroup> &CloneGroups, T C) {
+    C.constrain(CloneGroups);
----------------
Should this pair of functions be made static?


================
Comment at: include/clang/Analysis/CloneDetection.h:218-219
+///
+/// This class should be the base class of all constraints that are used in
+/// combination with the CloneDetector class.
+/// As constraints are specified in the form of template parameters, this class
----------------
While you could enforce this with `static_assert<is_base_of<CloneConstraint, T>>` in the detector, i think we shouldn't be strictly enforcing this.

Providing useful utility functions is pretty much the only purpose of this class, so it intends to be useful, but this shouldn't block users from implementing the `constrain()` method completely from scratch.

Also, we could probably move the method that groups clones by hashes here as the third utility method, if more than one hash function is eventually used(?)


================
Comment at: include/clang/Analysis/CloneDetection.h:318
+///
+/// This constraint searches for clones
+struct OnlyLargestCloneConstraint : CloneConstraint {
----------------
This comment looks incomplete.


================
Comment at: lib/Analysis/CloneDetection.cpp:308
+/// sequence in the \p Group.
+static bool containsAnyInGroup(StmtSequence &Stmt,
+                               CloneDetector::CloneGroup &Group) {
----------------
I remember some buildbots complained about conflicting names between variables and classes. Could we rename the `Stmt` parameter? Something like `Seq` is fine with me.


================
Comment at: lib/Analysis/CloneDetection.cpp:335-336
 
-    // Storage for the signatures of the direct child statements. This is only
-    // needed if the current statement is a CompoundStmt.
-    std::vector<CloneDetector::CloneSignature> ChildSignatures;
-    const CompoundStmt *CS = dyn_cast<const CompoundStmt>(S);
+void OnlyLargestCloneConstraint::constrain(
+    std::vector<CloneDetector::CloneGroup> &Result) {
+  std::vector<unsigned> IndexesToRemove;
----------------
In particular, this method doesn't use any `CloneConstraint`'s utility methods, as far as i understand.


================
Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:81-83
+  Detector.findClones(AllCloneGroups, RecursiveCloneTypeIIConstraint(),
+                      MinComplexityConstraint(MinComplexity),
+                      MinGroupSizeConstraint(2), OnlyLargestCloneConstraint());
----------------
Yay. I like how it looks.


https://reviews.llvm.org/D23418