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

[njames93]

Summary

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.

[njames93]

I've not added any specific unit tests for this, Don't think there would be much gained from them.
The init-variables and isolate-declaration checks do a good enough job of demonstrating the fix is working.
If needs must specific checks will be added.

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

16

I'll sort that out next diff

[njames93]

Fix new lines in test

[njames93]

Replace if/else logic with switches

[aaron.ballman]

LGTM with a few style nits.

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

629

I'd drop these return statements and the unreachable, below. The switch is fully-covered, so we'll get a diagnostic if any new enumerations are added and we forget to update this switch.

708

Similar here with continue and the unreachable.

[njames93]

Remove unreachable after switch

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

629

I'd still need a break anyway, but yes the unreachable probably isn't required

708

As this is in a loop, I'm torn whether using break makes it less readable. WDYT?

[aaron.ballman]

LG!

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

708

I tend to prefer break to continue because it's terribly easy to miss the fact that the switch statement is continuing the loop should someone decide they want to stick some code below the switch but within the loop for some reason.

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

void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
   // process, we maintain a counter with the amount of open intervals. If we
   // find an endpoint of an interval and this counter is different from 0, it
   // means that this interval overlaps with another one, so we set it as
   // inapplicable.
   struct Event {
     // An event can be either the begin or the end of an interval.
     enum EventType {
       ET_Begin = 1,
+      ET_Insert = 0,
       ET_End = -1,
     };
 
     Event(unsigned Begin, unsigned End, EventType Type, unsigned ErrorId,
           unsigned ErrorSize)
         : Type(Type), ErrorId(ErrorId) {
       // The events are going to be sorted by their position. In case of draw:
       //
Event(unsigned Begin, unsigned End, EventType Type, unsigned ErrorId,
       // * Finally, if we have two equal intervals whose errors have the same
       //   size, none of them will be strictly contained inside the other.
       //   Sorting by ErrorId will guarantee that the begin point of the first
       //   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)
         Priority = std::make_tuple(Begin, Type, -End, -ErrorSize, ErrorId);
+      else if (Type == ET_Insert)
+        Priority = std::make_tuple(Begin, Type, -End, ErrorSize, ErrorId);
       else
         Priority = std::make_tuple(End, Type, -Begin, ErrorSize, ErrorId);
     }
 
     bool operator<(const Event &Other) const {
       return Priority < Other.Priority;
     }
 
     // Determines if this event is the begin or the end of an interval.
for (const auto &ErrorAndFix : ErrorFixes) {
     for (const auto &FileAndReplaces : *ErrorAndFix.second) {
       for (const auto &Replace : FileAndReplaces.second)
         Size += Replace.getLength();
     }
     Sizes.push_back(Size);
   }
 
   // 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];
+        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]);
         }
       }
     }
+  }
 
   std::vector<bool> Apply(ErrorFixes.size(), true);
   for (auto &FileAndEvents : FileEvents) {
     std::vector<Event> &Events = FileAndEvents.second;
     // Sweep.
     llvm::sort(Events);
     int OpenIntervals = 0;
     for (const auto &Event : Events) {
+      if (Event.Type == Event::ET_Insert) {
+        if (OpenIntervals != 0)
+          Apply[Event.ErrorId] = false;
+        continue;
+      }
       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;
     }
     assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match");
   }
 
   for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
     if (!Apply[I]) {
       ErrorFixes[I].second->clear();
       ErrorFixes[I].first->Notes.emplace_back(
           "this fix will not be applied because it overlaps with another fix");
     }
   }
 }

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

+// 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;
+}