diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -24,6 +24,8 @@ #include "clang/Basic/DiagnosticOptions.h" #include "clang/Frontend/DiagnosticRenderer.h" #include "clang/Tooling/Core/Diagnostic.h" +#include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" #include @@ -589,8 +591,8 @@ }; Event(unsigned Begin, unsigned End, EventType Type, unsigned ErrorId, - unsigned ErrorSize) - : Type(Type), ErrorId(ErrorId) { + unsigned ErrorSize, const tooling::Replacement &Replacement) + : Type(Type), ErrorId(ErrorId), Replacement(&Replacement) { // The events are going to be sorted by their position. In case of draw: // // * If an interval ends at the same position at which other interval @@ -631,6 +633,8 @@ // The index of the error to which the interval that generated this event // belongs. unsigned ErrorId; + // The replacement this event relates to. + const tooling::Replacement *Replacement; // The events will be sorted based on this field. std::tuple Priority; }; @@ -666,15 +670,18 @@ 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]); + Events.emplace_back(Begin, End, Event::ET_Begin, I, Sizes[I], Replace); + Events.emplace_back(Begin, End, Event::ET_End, I, Sizes[I], Replace); } } } std::vector Apply(ErrorFixes.size(), true); for (auto &FileAndEvents : FileEvents) { + llvm::SmallSet RemoveOnlyEvents; std::vector &Events = FileAndEvents.second; + using ReplacementPtrSet = llvm::SmallDenseSet; + llvm::SmallDenseMap DiscardableReplacements; // Sweep. std::sort(Events.begin(), Events.end()); int OpenIntervals = 0; @@ -683,12 +690,66 @@ --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 (OpenIntervals != 0) { + if (Event.Replacement->getReplacementText().empty()) { + if (Event.Type == Event::ET_Begin) { + // Starting a removal only fix-it is OK inside another fix-it. + // But need to discard this specific fix-it from its parent so it + // wont get executed later and cause a conflict. + RemoveOnlyEvents.insert(&Event); + } else if (Event.Type == Event::ET_End) { + // End of a Remove only fix-it, Remove it from the set. + bool Found = false; + for (const auto *RemoveOnlyEvent : RemoveOnlyEvents) { + if (RemoveOnlyEvent->Replacement == Event.Replacement) { + assert(!Found && "Event should only appear in set once"); + RemoveOnlyEvents.erase(RemoveOnlyEvent); + Found = true; + } + } + DiscardableReplacements[Event.ErrorId].insert(Event.Replacement); + assert(Found && "Event didn't appear in set"); + } + } else { + // This isnt a text removal only change, so must be a conflict. + Apply[Event.ErrorId] = false; + } + } else { + decltype(RemoveOnlyEvents)::size_type Size = RemoveOnlyEvents.size(); + assert(Size < 2 && "Once OpenIntervals is `0` this set should contain " + "no more than 1 event"); + if (Size) { + // The remove only fix-it is overlapping another even that we have + // already disabled. So no need to discard this fix-it + RemoveOnlyEvents.clear(); + } + } if (Event.Type == Event::ET_Begin) ++OpenIntervals; } assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match"); + for (const auto &ErrorAndDiscarded : DiscardableReplacements) { + unsigned ErrorIndex = ErrorAndDiscarded.first; + const ReplacementPtrSet &Discarded = ErrorAndDiscarded.second; + if (!Apply[ErrorIndex]) + continue; // The whole error has already been discarded. + tooling::Replacements NewReplacements; + const tooling::Replacements &CurReplacements = + ErrorFixes[ErrorIndex].second->lookup(FileAndEvents.first); + for (const tooling::Replacement &Replacement : CurReplacements) { + // Comparing the pointer here is fine as they are pointing to the same + // Replacement. + if (Discarded.count(&Replacement)) + continue; + if (NewReplacements.add(Replacement)) { + // This should never fire as we have just tested + // but leave the check in just in case. + Apply[ErrorIndex] = false; + } + } + ErrorFixes[ErrorIndex].second->insert_or_assign( + FileAndEvents.first, std::move(NewReplacements)); + } } for (unsigned I = 0; I < ErrorFixes.size(); ++I) {