njames93 updated this revision to Diff 240163.
njames93 added a comment.
- Small edge case tweak
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73203/new/
https://reviews.llvm.org/D73203
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ 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 <tuple>
@@ -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<unsigned, EventType, int, int, unsigned> 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<bool> Apply(ErrorFixes.size(), true);
for (auto &FileAndEvents : FileEvents) {
+ llvm::SmallSet<const Event *, 4> RemoveOnlyEvents;
std::vector<Event> &Events = FileAndEvents.second;
+ using ReplacementPtrSet = llvm::SmallDenseSet<const tooling::Replacement *>;
+ llvm::SmallDenseMap<unsigned, ReplacementPtrSet> 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) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
