On Fri, Oct 16, 2015 at 4:43 AM, Angel Garcia Gomez via cfe-commits < cfe-commits@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
