[clang-tools-extra] r250509 - Fix overlapping replacements in clang-tidy.
Angel Garcia Gomez via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 16 04:43:50 PDT 2015
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 {
- 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 {
+ 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
More information about the cfe-commits
mailing list