[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
JonasToth added a comment. We need tests for that. There should be tests for the exporting-feature. Maybe they could provide a good starting point on adding tests. (not sure if there is unit-test code for isolated testing, but some tests do exist!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73203/new/ https://reviews.llvm.org/D73203 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
merge_guards_bot added a comment.
{icon times-circle color=red} Unit tests: fail. 62126 tests passed, 5 failed
and 807 were skipped.
failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
{icon check-circle color=green} clang-tidy: pass.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73203/new/
https://reviews.llvm.org/D73203
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
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
@@ -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].sec
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62056 tests passed, 0 failed
and 783 were skipped.
{icon question-circle color=gray} clang-tidy: unknown.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73203/new/
https://reviews.llvm.org/D73203
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
njames93 updated this revision to Diff 239636.
njames93 marked 3 inline comments as done.
njames93 added a comment.
- remove unnecessary auto
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
@@ -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,60 @@
--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);
+DiscardableReplacements[Event.ErrorId].insert(Event.Replacement);
+ } 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->ErrorId == Event.ErrorId) {
+assert(!Found && "Event should only appear in set once");
+RemoveOnlyEvents.erase(RemoveOnlyEvent);
+Found = true;
+ }
+}
+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 {
+assert(RemoveOnlyEvents.empty() &&
+ "Once OpenIntervals is `0` this set should be empty");
+ }
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))
+ continu
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:726 + unsigned ErrorIndex = ErrorAndDiscarded.first; + const auto &Discarded = ErrorAndDiscarded.second; + if (!Apply[ErrorIndex]) Eugene.Zelenko wrote: > Please don't use auto unless type is spelled in same statement or iterator. What if the type is ``llvm::SmallDenseSet >`` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73203/new/ https://reviews.llvm.org/D73203 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:726 + unsigned ErrorIndex = ErrorAndDiscarded.first; + const auto &Discarded = ErrorAndDiscarded.second; + if (!Apply[ErrorIndex]) Please don't use auto unless type is spelled in same statement or iterator. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:730 + tooling::Replacements NewReplacements; + const auto &CurReplacements = + ErrorFixes[ErrorIndex].second->lookup(FileAndEvents.first); Please don't use auto unless type is spelled in same statement or iterator. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73203/new/ https://reviews.llvm.org/D73203 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62056 tests passed, 0 failed
and 783 were skipped.
{icon question-circle color=gray} clang-tidy: unknown.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73203/new/
https://reviews.llvm.org/D73203
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73203: [clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit
njames93 created this revision.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
Repository:
rG LLVM Github Monorepo
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
@@ -589,8 +589,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 +631,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 +668,19 @@
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;
+llvm::SmallDenseMap>
+DiscardableReplacements;
// Sweep.
std::sort(Events.begin(), Events.end());
int OpenIntervals = 0;
@@ -683,12 +689,60 @@
--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);
+DiscardableReplacements[Event.ErrorId].insert(Event.Replacement);
+ } 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->ErrorId == Event.ErrorId) {
+assert(!Found && "Event should only appear in set once");
+RemoveOnlyEvents.erase(RemoveOnlyEvent);
+Found = true;
+ }
+}
+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 {
+assert(RemoveOnlyEvents.empty() &&
+ "Once OpenIntervals is `0` this set should be empty");
+ }
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 auto &Discarded = ErrorAndDiscarded.second;
+ if (!Apply[ErrorIndex])
+continue; // The whole error has already been discarded
+ tooling::Replacements NewReplacements;
+ const auto &CurReplacements =
+ ErrorFixes[ErrorIndex].second->lookup(FileAndEvents.first);
+ for (const auto &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
cfe-commits@lists.l
