[clang-tools-extra] r250509 - Fix overlapping replacements in clang-tidy.
Angel Garcia via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 19 02:26:15 PDT 2015
Using these as the default comparison operators might not make much sense,
as they don't take into account all the fields (they only look at
ClangTidyError::Message). But here I just wanted to mimic existing
behavior, so honestly I don't know.
I implemented equality with !<&&!< to improve maintainability. If someone
ever wants to modify LessClangTidyError, EqualClangTidyError will still be
consistent with the new definition without any additional work. Moreover,
LessClangTidyError is used to sort, while EqualClangTidyError is used by
std::unique (a linear amount of times), so the extra work shouldn't be too
bad for the performance.
On Fri, Oct 16, 2015 at 10:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Oct 16, 2015 at 4:43 AM, Angel Garcia Gomez via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: angelgarcia
>> Date: Fri Oct 16 06:43:49 2015
>> New Revision: 250509
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=250509&view=rev
>> Log:
>> Fix overlapping replacements in clang-tidy.
>>
>> Summary: Prevent clang-tidy from applying fixes to errors that overlap
>> with other errors' fixes, with one exception: if one fix is completely
>> contained inside another one, then we can apply the big one.
>>
>> Reviewers: bkramer, klimek
>>
>> Subscribers: djasper, cfe-commits, alexfh
>>
>> Differential Revision: http://reviews.llvm.org/D13516
>>
>> Modified:
>> clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
>> clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
>>
>> clang-tools-extra/trunk/unittests/clang-tidy/OverlappingReplacementsTest.cpp
>>
>> Modified:
>> clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=250509&r1=250508&r2=250509&view=diff
>>
>> ==============================================================================
>> --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
>> (original)
>> +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
>> Fri Oct 16 06:43:49 2015
>> @@ -22,8 +22,8 @@
>> #include "clang/Basic/DiagnosticOptions.h"
>> #include "clang/Frontend/DiagnosticRenderer.h"
>> #include "llvm/ADT/SmallString.h"
>> -#include <set>
>> #include <tuple>
>> +#include <vector>
>> using namespace clang;
>> using namespace tidy;
>>
>> @@ -146,8 +146,7 @@ static llvm::Regex ConsumeGlob(StringRef
>> }
>>
>> GlobList::GlobList(StringRef Globs)
>> - : Positive(!ConsumeNegativeIndicator(Globs)),
>> - Regex(ConsumeGlob(Globs)),
>> + : Positive(!ConsumeNegativeIndicator(Globs)),
>> Regex(ConsumeGlob(Globs)),
>> NextGlob(Globs.empty() ? nullptr : new GlobList(Globs)) {}
>>
>> bool GlobList::contains(StringRef S, bool Contains) {
>> @@ -222,9 +221,7 @@ const ClangTidyOptions &ClangTidyContext
>> return CurrentOptions;
>> }
>>
>> -void ClangTidyContext::setCheckProfileData(ProfileData *P) {
>> - Profile = P;
>> -}
>> +void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile =
>> P; }
>>
>> GlobList &ClangTidyContext::getChecksFilter() {
>> assert(CheckFilter != nullptr);
>> @@ -296,16 +293,16 @@ void ClangTidyDiagnosticConsumer::Handle
>> // This is a compiler diagnostic without a warning option. Assign
>> check
>> // name based on its level.
>> switch (DiagLevel) {
>> - case DiagnosticsEngine::Error:
>> - case DiagnosticsEngine::Fatal:
>> - CheckName = "clang-diagnostic-error";
>> - break;
>> - case DiagnosticsEngine::Warning:
>> - CheckName = "clang-diagnostic-warning";
>> - break;
>> - default:
>> - CheckName = "clang-diagnostic-unknown";
>> - break;
>> + case DiagnosticsEngine::Error:
>> + case DiagnosticsEngine::Fatal:
>> + CheckName = "clang-diagnostic-error";
>> + break;
>> + case DiagnosticsEngine::Warning:
>> + CheckName = "clang-diagnostic-warning";
>> + break;
>> + default:
>> + CheckName = "clang-diagnostic-unknown";
>> + break;
>> }
>> }
>>
>> @@ -340,7 +337,7 @@ bool ClangTidyDiagnosticConsumer::passes
>> unsigned LineNumber)
>> const {
>> if (Context.getGlobalOptions().LineFilter.empty())
>> return true;
>> - for (const FileFilter& Filter : Context.getGlobalOptions().LineFilter)
>> {
>> + for (const FileFilter &Filter : Context.getGlobalOptions().LineFilter)
>> {
>> if (FileName.endswith(Filter.Name)) {
>> if (Filter.LineRanges.empty())
>> return true;
>> @@ -398,26 +395,147 @@ llvm::Regex *ClangTidyDiagnosticConsumer
>> return HeaderFilter.get();
>> }
>>
>> +void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
>> + SmallVectorImpl<ClangTidyError> &Errors) const {
>> + // Each error is modelled as the set of intervals in which it applies
>> + // replacements. To detect overlapping replacements, we use a sweep
>> line
>> + // algorithm over these sets of intervals.
>> + // An event here consists of the opening or closing of an interval.
>> During the
>> + // proccess, 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_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:
>> + //
>> + // * If an interval ends at the same position at which other
>> interval
>> + // begins, this is not an overlapping, so we want to remove the
>> ending
>> + // interval before adding the starting one: end events have
>> higher
>> + // priority than begin events.
>> + //
>> + // * If we have several begin points at the same position, we will
>> mark as
>> + // inapplicable the ones that we proccess later, so the first
>> one has to
>> + // be the one with the latest end point, because this one will
>> contain
>> + // all the other intervals. For the same reason, if we have
>> several end
>> + // points in the same position, the last one has to be the one
>> with the
>> + // earliest begin point. In both cases, we sort non-increasingly
>> by the
>> + // position of the complementary.
>> + //
>> + // * In case of two equal intervals, the one whose error is bigger
>> can
>> + // potentially contain the other one, so we want to proccess its
>> begin
>> + // points before and its end points later.
>> + //
>> + // * 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 proccessed before, disallowing the second one,
>> and the
>> + // end point of the first one will also be proccessed before,
>> + // disallowing the first one.
>> + if (Type == ET_Begin)
>> + 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.
>> + EventType Type;
>> + // The index of the error to which the interval that generated this
>> event
>> + // belongs.
>> + unsigned ErrorId;
>> + // The events will be sorted based on this field.
>> + std::tuple<unsigned, EventType, int, int, unsigned> Priority;
>> + };
>> +
>> + // Compute error sizes.
>> + std::vector<int> Sizes;
>> + for (const auto &Error : Errors) {
>> + int Size = 0;
>> + for (const auto &Replace : Error.Fix)
>> + Size += Replace.getLength();
>> + Sizes.push_back(Size);
>> + }
>> +
>> + // Build events from error intervals.
>> + std::vector<Event> Events;
>> + for (unsigned I = 0; I < Errors.size(); ++I) {
>> + for (const auto &Replace : Errors[I].Fix) {
>> + unsigned Begin = Replace.getOffset();
>> + unsigned End = Begin + Replace.getLength();
>> + // FIXME: Handle empty intervals, such as those from insertions.
>> + if (Begin == End)
>> + continue;
>> + Events.push_back(Event(Begin, End, Event::ET_Begin, I, Sizes[I]));
>> + Events.push_back(Event(Begin, End, Event::ET_End, I, Sizes[I]));
>> + }
>> + }
>> + std::sort(Events.begin(), Events.end());
>> +
>> + // Sweep.
>> + std::vector<bool> Apply(Errors.size(), true);
>> + 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;
>> + }
>> + assert(OpenIntervals == 0 && "Amount of begin/end points doesn't
>> match");
>> +
>> + for (unsigned I = 0; I < Errors.size(); ++I) {
>> + if (!Apply[I]) {
>> + Errors[I].Fix.clear();
>> + Errors[I].Notes.push_back(
>> + ClangTidyMessage("this fix will not be applied because"
>> + " it overlaps with another fix"));
>> + }
>> + }
>> +}
>> +
>> namespace {
>> struct LessClangTidyError {
>>
>
> Should this just be op< on ClangTidyError?
>
>
>> - bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS)
>> const {
>> - const ClangTidyMessage &M1 = LHS->Message;
>> - const ClangTidyMessage &M2 = RHS->Message;
>> + bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS)
>> const {
>> + const ClangTidyMessage &M1 = LHS.Message;
>> + const ClangTidyMessage &M2 = RHS.Message;
>>
>> return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
>> std::tie(M2.FilePath, M2.FileOffset, M2.Message);
>
> }
>> };
>> +struct EqualClangTidyError {
>>
>
> & this op== on ClangTidyError? (I'd probably define it separately, with
> another tie, rather than using !<&&!< to reduce the work involved in
> equality testing)
>
> Or do both of these operations represent truely non-inherent
> equality/ordering of ClangTidyErrors? (ie: they don't make sense as the
> defaults, but they make sense in this particular instance)
>
>
>> + static LessClangTidyError Less;
>> + bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS)
>> const {
>> + return !Less(LHS, RHS) && !Less(RHS, LHS);
>> + }
>> +};
>> } // end anonymous namespace
>>
>> // Flushes the internal diagnostics buffer to the ClangTidyContext.
>> void ClangTidyDiagnosticConsumer::finish() {
>> finalizeLastError();
>> - std::set<const ClangTidyError*, LessClangTidyError> UniqueErrors;
>> - for (const ClangTidyError &Error : Errors)
>> - UniqueErrors.insert(&Error);
>>
>> - for (const ClangTidyError *Error : UniqueErrors)
>> - Context.storeError(*Error);
>> + std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
>> + Errors.erase(std::unique(Errors.begin(), Errors.end(),
>> EqualClangTidyError()),
>> + Errors.end());
>> + removeIncompatibleErrors(Errors);
>> +
>> + for (const ClangTidyError &Error : Errors)
>> + Context.storeError(Error);
>> Errors.clear();
>> }
>>
>> Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=250509&r1=250508&r2=250509&view=diff
>>
>> ==============================================================================
>> --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
>> (original)
>> +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h Fri
>> Oct 16 06:43:49 2015
>> @@ -179,8 +179,8 @@ public:
>> ///
>> /// Setting a non-null pointer here will enable profile collection in
>> /// clang-tidy.
>> - void setCheckProfileData(ProfileData* Profile);
>> - ProfileData* getCheckProfileData() const { return Profile; }
>> + void setCheckProfileData(ProfileData *Profile);
>> + ProfileData *getCheckProfileData() const { return Profile; }
>>
>> private:
>> // Calls setDiagnosticsEngine() and storeError().
>> @@ -231,9 +231,11 @@ public:
>> private:
>> void finalizeLastError();
>>
>> + void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors)
>> const;
>> +
>> /// \brief Returns the \c HeaderFilter constructed for the options set
>> in the
>> /// context.
>> - llvm::Regex* getHeaderFilter();
>> + llvm::Regex *getHeaderFilter();
>>
>> /// \brief Updates \c LastErrorRelatesToUserCode and
>> LastErrorPassesLineFilter
>> /// according to the diagnostic \p Location.
>>
>> Modified:
>> clang-tools-extra/trunk/unittests/clang-tidy/OverlappingReplacementsTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/OverlappingReplacementsTest.cpp?rev=250509&r1=250508&r2=250509&view=diff
>>
>> ==============================================================================
>> ---
>> clang-tools-extra/trunk/unittests/clang-tidy/OverlappingReplacementsTest.cpp
>> (original)
>> +++
>> clang-tools-extra/trunk/unittests/clang-tidy/OverlappingReplacementsTest.cpp
>> Fri Oct 16 06:43:49 2015
>> @@ -77,11 +77,12 @@ public:
>> auto *VD = Result.Nodes.getNodeAs<VarDecl>(BoundDecl);
>> std::string NewName = newName(VD->getName());
>>
>> - auto Diag = diag(VD->getLocation(), "refactor")
>> + auto Diag = diag(VD->getLocation(), "refactor %0 into %1")
>> + << VD->getName() << NewName
>> << FixItHint::CreateReplacement(
>> - CharSourceRange::getTokenRange(VD->getLocation(),
>> - VD->getLocation()),
>> - NewName);
>> + CharSourceRange::getTokenRange(VD->getLocation(),
>> + VD->getLocation()),
>> + NewName);
>>
>> class UsageVisitor : public RecursiveASTVisitor<UsageVisitor> {
>> public:
>> @@ -281,7 +282,7 @@ TEST(OverlappingReplacementsTest, Replac
>>
>> // Apply the UseCharCheck together with the IfFalseCheck.
>> //
>> - // The 'If' fix is bigger, so that is the one that has to be applied.
>> + // The 'If' fix contains the other, so that is the one that has to be
>> applied.
>> // } else if (int a = 0) {
>> // ^^^ -> char
>> // ~~~~~~~~~ -> false
>> @@ -294,7 +295,9 @@ TEST(OverlappingReplacementsTest, Replac
>> }
>> })";
>> Res = runCheckOnCode<UseCharCheck, IfFalseCheck>(Code);
>> - // FIXME: EXPECT_EQ(CharIfFix, Res);
>> + EXPECT_EQ(CharIfFix, Res);
>> + Res = runCheckOnCode<IfFalseCheck, UseCharCheck>(Code);
>> + EXPECT_EQ(CharIfFix, Res);
>>
>> // Apply the IfFalseCheck with the StartsWithPotaCheck.
>> //
>> @@ -303,7 +306,7 @@ TEST(OverlappingReplacementsTest, Replac
>> // ^^^^^^ -> tomato
>> // ~~~~~~~~~~~~~~~ -> false
>> //
>> - // But the refactoring is bigger here:
>> + // But the refactoring is the one that contains the other here:
>> // char potato = 0;
>> // ^^^^^^ -> tomato
>> // if (potato) potato;
>> @@ -318,60 +321,87 @@ TEST(OverlappingReplacementsTest, Replac
>> }
>> })";
>> Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
>> - // FIXME: EXPECT_EQ(IfStartsFix, Res);
>> -
>> - // Silence warnings.
>> - (void)CharIfFix;
>> - (void)IfStartsFix;
>> + EXPECT_EQ(IfStartsFix, Res);
>> + Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck>(Code);
>> + EXPECT_EQ(IfStartsFix, Res);
>> }
>>
>> -TEST(OverlappingReplacementsTest,
>> ApplyFullErrorOrNothingWhenOverlapping) {
>> +TEST(OverlappingReplacements, TwoReplacementsInsideOne) {
>> std::string Res;
>> const char Code[] =
>> R"(void f() {
>> - int potato = 0;
>> - potato += potato * potato;
>> - if (char this_name_make_this_if_really_long = potato) potato;
>> + if (int potato = 0) {
>> + int a = 0;
>> + }
>> })";
>>
>> - // StartsWithPotaCheck will try to refactor 'potato' into 'tomato',
>> - // and EndsWithTatoCheck will try to use 'pomelo'. We have to apply
>> - // either all conversions from one check, or all from the other.
>> - const char StartsFix[] =
>> + // The two smallest replacements should not be applied.
>> + // if (int potato = 0) {
>> + // ^^^^^^ -> tomato
>> + // *** -> char
>> + // ~~~~~~~~~~~~~~ -> false
>> + // But other errors from the same checks should not be affected.
>> + // int a = 0;
>> + // *** -> char
>> + const char Fix[] =
>> R"(void f() {
>> - int tomato = 0;
>> - tomato += tomato * tomato;
>> - if (char this_name_make_this_if_really_long = tomato) tomato;
>> + if (false) {
>> + char a = 0;
>> + }
>> })";
>> - const char EndsFix[] =
>> + Res = runCheckOnCode<UseCharCheck, IfFalseCheck,
>> StartsWithPotaCheck>(Code);
>> + EXPECT_EQ(Fix, Res);
>> + Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck,
>> UseCharCheck>(Code);
>> + EXPECT_EQ(Fix, Res);
>> +}
>> +
>> +TEST(OverlappingReplacementsTest,
>> + ApplyAtMostOneOfTheChangesWhenPartialOverlapping) {
>> + std::string Res;
>> + const char Code[] =
>> R"(void f() {
>> - int pomelo = 0;
>> - pomelo += pomelo * pomelo;
>> - if (char this_name_make_this_if_really_long = pomelo) pomelo;
>> + if (int potato = 0) {
>> + int a = potato;
>> + }
>> })";
>> - // In case of overlapping, we will prioritize the biggest fix.
>> However, these
>> - // two fixes have the same size and position, so we don't know yet
>> which one
>> - // will have preference.
>> - Res = runCheckOnCode<StartsWithPotaCheck, EndsWithTatoCheck>(Code);
>> - // FIXME: EXPECT_TRUE(Res == StartsFix || Res == EndsFix);
>>
>> - // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', but
>> - // replacing the 'if' condition is a bigger change than all the
>> refactoring
>> - // changes together (48 vs 36), so this is the one that is going to be
>> - // applied.
>> + // These two replacements overlap, but none of them is completely
>> contained
>> + // inside the other.
>> + // if (int potato = 0) {
>> + // ^^^^^^ -> tomato
>> + // ~~~~~~~~~~~~~~ -> false
>> + // int a = potato;
>> + // ^^^^^^ -> tomato
>> + //
>> + // The 'StartsWithPotaCheck' fix has endpoints inside the
>> 'IfFalseCheck' fix,
>> + // so it is going to be set as inapplicable. The 'if' fix will be
>> applied.
>> const char IfFix[] =
>> R"(void f() {
>> + if (false) {
>> + int a = potato;
>> + }
>> +})";
>> + Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
>> + EXPECT_EQ(IfFix, Res);
>> +}
>> +
>> +TEST(OverlappingReplacementsTest, TwoErrorsHavePerfectOverlapping) {
>> + std::string Res;
>> + const char Code[] =
>> + R"(void f() {
>> int potato = 0;
>> potato += potato * potato;
>> - if (true) potato;
>> + if (char a = potato) potato;
>> })";
>> - Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck>(Code);
>> - // FIXME: EXPECT_EQ(IfFix, Res);
>>
>> - // Silence warnings.
>> - (void)StartsFix;
>> - (void)EndsFix;
>> - (void)IfFix;
>> + // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', and
>> + // EndsWithTatoCheck will try to use 'pomelo'. Both fixes have the
>> same set of
>> + // ranges. This is a corner case of one error completely containing
>> another:
>> + // the other completely contains the first one as well. Both errors are
>> + // discarded.
>> +
>> + Res = runCheckOnCode<StartsWithPotaCheck, EndsWithTatoCheck>(Code);
>> + EXPECT_EQ(Code, Res);
>> }
>>
>> } // namespace test
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151019/90a3f54f/attachment-0001.html>
More information about the cfe-commits
mailing list