[clang-tools-extra] r250509 - Fix overlapping replacements in clang-tidy.
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 16 13:50:36 PDT 2015
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/20151016/1e98d557/attachment-0001.html>
More information about the cfe-commits
mailing list