NoQ added a comment.
I couldn't find any real problems - the code's getting mature! Please accept a
few very important "please rename this variable" comments :)
================
Comment at: include/clang/Analysis/CloneDetection.h:236
@@ +235,3 @@
+ /// clone in this pair.
+ struct SuspiciousClone {
+ /// The variable which referencing in this clone was against the pattern.
----------------
It surprised me that you cannot really retrieve the clone (i.e. statement
sequence) from the `SuspiciousClone` object. Maybe rename to eg.
`SuspiciousCloneInfo`?
================
Comment at: lib/Analysis/CloneDetection.cpp:92
@@ -91,1 +91,3 @@
+ /// The location in the code where the variable was references.
+ SourceRange Location;
----------------
It's surprising that `Location` is not of type `SourceLocation`, so much that
i'd agree to rename it to `Range`, no matter how generic this word is.
================
Comment at: lib/Analysis/CloneDetection.cpp:174
@@ -165,2 +173,3 @@
/// pattern and the statements of this objects are clones of each other.
- bool comparePattern(const VariablePattern &Other) {
+ unsigned getPatternDifferences(
+ const VariablePattern &Other,
----------------
Because this function doesn't return pattern differences, maybe rename to
`countPatternDifferences()`? And the optional parameter kinda comes as a bonus
feature.
================
Comment at: lib/Analysis/CloneDetection.cpp:183
@@ +182,3 @@
+ auto OtherOccurence = Other.Occurences[i];
+ if (ThisOccurence.KindID != OtherOccurence.KindID) {
+ ++NumberOfDifferences;
----------------
Maybe reduce indent here through `if (... == ...) continue`, like in other
places?
================
Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:39
@@ +38,3 @@
+ /// \brief Reports all clones to the user.
+ void reportClones(SourceManager &SM, AnalysisManager &Mgr,
+ int MinComplexity) const {
----------------
Maybe move those out-of-line? They look pretty big.
================
Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:43
@@ +42,3 @@
+ std::vector<CloneDetector::CloneGroup> CloneGroups;
+ CloneDetector.findClones(CloneGroups, MinComplexity);
+
----------------
I renamed the member variable to `Detector` in r276791, because it caused
compile errors on buildbots (because class and member have the same name), so
this patch would need to be patched up a bit - here and in one more place.
================
Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:97
@@ +96,3 @@
+ DiagEngine.Report(Pair.FirstClone.Location.getBegin(), WarnID)
+ << Pair.FirstClone.Location << Pair.FirstClone.Suggestion;
+
----------------
This line probably explains why i'm so much into renaming variables: it reads
as if we're putting the location of the clone into the report, even though
we're putting the location of a variable reference into it.
`Pair.FirstCloneInfo.VarRange` would have been much more intuitive.
https://reviews.llvm.org/D23314
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
