[PATCH] D23314: [analyzer] CloneDetector allows comparing clones for suspicious variable pattern errors.

[Artem Dergachev via cfe-commits]

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