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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
