[clang-tools-extra] bbc2dde - [clang-tidy] Handled insertion only fixits when determining conflicts.

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2020-07-29T16:35:44+01:00
New Revision: bbc2ddecbd342d4502fe43466bd3658b89ddad7d

URL: https://github.com/llvm/llvm-project/commit/bbc2ddecbd342d4502fe43466bd3658b89ddad7d
DIFF: https://github.com/llvm/llvm-project/commit/bbc2ddecbd342d4502fe43466bd3658b89ddad7d.diff

LOG: [clang-tidy] Handled insertion only fixits when determining conflicts.

Handle insertion fix-its when removing incompatible errors by introducting a new EventType `ET_Insert`
This has lower prioirty than End events, but higher than begin.
Idea being If an insert is at the same place as a begin event, the insert should be processed first to reduce unnecessary conflicts.
Likewise if its at the same place as an end event, process the end event first for the same reason.

This also fixes https://bugs.llvm.org/show_bug.cgi?id=46511.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D82898

Added: 
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables-conflict.cpp

Modified: 
    clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 521e6ef549b9..1471301a3431 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -31,6 +31,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Regex.h"
 #include <tuple>
@@ -590,6 +591,7 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
     // An event can be either the begin or the end of an interval.
     enum EventType {
       ET_Begin = 1,
+      ET_Insert = 0,
       ET_End = -1,
     };
 
@@ -621,10 +623,17 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
       //   one will be processed before, disallowing the second one, and the
       //   end point of the first one will also be processed before,
       //   disallowing the first one.
-      if (Type == ET_Begin)
+      switch (Type) {
+      case ET_Begin:
         Priority = std::make_tuple(Begin, Type, -End, -ErrorSize, ErrorId);
-      else
+        break;
+      case ET_Insert:
+        Priority = std::make_tuple(Begin, Type, -End, ErrorSize, ErrorId);
+        break;
+      case ET_End:
         Priority = std::make_tuple(End, Type, -Begin, ErrorSize, ErrorId);
+        break;
+      }
     }
 
     bool operator<(const Event &Other) const {
@@ -662,19 +671,19 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
   }
 
   // Build events from error intervals.
-  std::map<std::string, std::vector<Event>> FileEvents;
+  llvm::StringMap<std::vector<Event>> FileEvents;
   for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
     for (const auto &FileAndReplace : *ErrorFixes[I].second) {
       for (const auto &Replace : FileAndReplace.second) {
         unsigned Begin = Replace.getOffset();
         unsigned End = Begin + Replace.getLength();
-        const std::string &FilePath = std::string(Replace.getFilePath());
-        // FIXME: Handle empty intervals, such as those from insertions.
-        if (Begin == End)
-          continue;
-        auto &Events = FileEvents[FilePath];
-        Events.emplace_back(Begin, End, Event::ET_Begin, I, Sizes[I]);
-        Events.emplace_back(Begin, End, Event::ET_End, I, Sizes[I]);
+        auto &Events = FileEvents[Replace.getFilePath()];
+        if (Begin == End) {
+          Events.emplace_back(Begin, End, Event::ET_Insert, I, Sizes[I]);
+        } else {
+          Events.emplace_back(Begin, End, Event::ET_Begin, I, Sizes[I]);
+          Events.emplace_back(Begin, End, Event::ET_End, I, Sizes[I]);
+        }
       }
     }
   }
@@ -686,14 +695,20 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
     llvm::sort(Events);
     int OpenIntervals = 0;
     for (const auto &Event : Events) {
-      if (Event.Type == Event::ET_End)
-        --OpenIntervals;
-      // This has to be checked after removing the interval from the count if it
-      // is an end event, or before adding it if it is a begin event.
-      if (OpenIntervals != 0)
-        Apply[Event.ErrorId] = false;
-      if (Event.Type == Event::ET_Begin)
-        ++OpenIntervals;
+      switch (Event.Type) {
+      case Event::ET_Begin:
+        if (OpenIntervals++ != 0)
+          Apply[Event.ErrorId] = false;
+        break;
+      case Event::ET_Insert:
+        if (OpenIntervals != 0)
+          Apply[Event.ErrorId] = false;
+        break;
+      case Event::ET_End:
+        if (--OpenIntervals != 0)
+          Apply[Event.ErrorId] = false;
+        break;
+      }
     }
     assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match");
   }

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables-conflict.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables-conflict.cpp
new file mode 100644
index 000000000000..4d67feb4161f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables-conflict.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables,readability-isolate-declaration %t
+
+void foo() {
+  int A, B, C;
+  // CHECK-MESSAGES-DAG: :[[@LINE-1]]:7: warning: variable 'A' is not initialized
+  // CHECK-MESSAGES-DAG: :[[@LINE-2]]:10: warning: variable 'B' is not initialized
+  // CHECK-MESSAGES-DAG: :[[@LINE-3]]:13: warning: variable 'C' is not initialized
+  // CHECK-MESSAGES-DAG: :[[@LINE-4]]:3: warning: multiple declarations in a single statement reduces readability
+
+  // Only the isolate declarations fix-it should be applied
+
+  //      CHECK-FIXES: int A;
+  // CHECK-FIXES-NEXT: int B;
+  // CHECK-FIXES-NEXT: int C;
+}