angelgarcia updated this revision to Diff 37323.
angelgarcia added a comment.
Fix comments.
http://reviews.llvm.org/D13516
Files:
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
unittests/clang-tidy/OverlappingReplacementsTest.cpp
Index: unittests/clang-tidy/OverlappingReplacementsTest.cpp
===================================================================
--- unittests/clang-tidy/OverlappingReplacementsTest.cpp
+++ unittests/clang-tidy/OverlappingReplacementsTest.cpp
@@ -77,11 +77,12 @@
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 @@
// 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,16 +295,16 @@
}
})";
Res = runCheckOnCode<UseCharCheck, IfFalseCheck>(Code);
- // FIXME: EXPECT_EQ(CharIfFix, Res);
+ EXPECT_EQ(CharIfFix, Res);
// Apply the IfFalseCheck with the StartsWithPotaCheck.
//
// The 'If' replacement is bigger here.
// if (char potato = 0) {
// ^^^^^^ -> 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 +319,73 @@
}
})";
Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
- // FIXME: EXPECT_EQ(IfStartsFix, Res);
-
- // Silence warnings.
- (void)CharIfFix;
- (void)IfStartsFix;
+ EXPECT_EQ(IfStartsFix, Res);
}
-TEST(OverlappingReplacementsTest, ApplyFullErrorOrNothingWhenOverlapping) {
- std::string Res;
+TEST(OverlappingReplacements, TwoReplacementsInsideOne) {
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[] =
+ std::string Res =
+ runCheckOnCode<UseCharCheck, IfFalseCheck, StartsWithPotaCheck>(Code);
+ EXPECT_EQ(Fix, Res);
+}
+
+TEST(OverlappingReplacementsTest, DiscardBothChangesWhenPartialOverlapping) {
+ 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.
- const char IfFix[] =
+ // These two replacements overlap, but none of them is completely contained
+ // inside the other. Both of them are discarded.
+ // if (int potato = 0) {
+ // ^^^^^^ -> tomato
+ // ~~~~~~~~~~~~~~ -> false
+ // int a = potato;
+ // ^^^^^^ -> tomato
+ std::string Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
+ EXPECT_EQ(Code, 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);
+ Res = runCheckOnCode<EndsWithTatoCheck, StartsWithPotaCheck>(Code);
+ EXPECT_EQ(Code, Res);
}
} // namespace test
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -179,8 +179,8 @@
///
/// 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,21 @@
private:
void finalizeLastError();
+ using ErrorInterval = std::pair<unsigned, unsigned>;
+ using ErrorIntervals = std::vector<ErrorInterval>;
+ enum OverlappingKind {
+ OK_Disjoint,
+ OK_FirstInsideSecond,
+ OK_SecondInsideFirst,
+ OK_Overlap,
+ };
+ OverlappingKind getOverlappingKind(const ErrorIntervals &First,
+ const ErrorIntervals &Second) const;
+ 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.
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -22,6 +22,7 @@
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Frontend/DiagnosticRenderer.h"
#include "llvm/ADT/SmallString.h"
+#include <queue>
#include <set>
#include <tuple>
using namespace clang;
@@ -146,8 +147,7 @@
}
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 +222,7 @@
return CurrentOptions;
}
-void ClangTidyContext::setCheckProfileData(ProfileData *P) {
- Profile = P;
-}
+void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile = P; }
GlobList &ClangTidyContext::getChecksFilter() {
assert(CheckFilter != nullptr);
@@ -296,16 +294,16 @@
// 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 +338,7 @@
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 +396,144 @@
return HeaderFilter.get();
}
+ClangTidyDiagnosticConsumer::OverlappingKind
+ClangTidyDiagnosticConsumer::getOverlappingKind(
+ const ErrorIntervals &First, const ErrorIntervals &Second) const {
+ // We are using a sweep line algorithm over both sets of intervals. An event
+ // here consists of the opening or closing of an interval from any of the
+ // two sets, and at which position. This way, for each range between two
+ // events, we can know if there are intervals from a concrete set that are
+ // covering that range.
+ //
+ // All the events in the same position are handled together. This way, if
+ // one of the sets has two consecutive intervals (like "[a, b)[b, c)"), we
+ // won't erroneously think that there is a hole in the middle.
+ //
+ // FIXME: This approach doesn't cope well with empty intervals (such as those
+ // from insertions). For now, we are blind to them, but we could pretend them
+ // to be one-unit length intervals to check out how this works.
+ struct Event {
+ bool operator<(const Event &E) const { return Pos < E.Pos; }
+ // File offset at which this event happens.
+ unsigned Pos;
+ // This event represents the beggining or the ending of an interval (1 or
+ // -1 respectively).
+ int Type;
+ // Identifies the error (0 or 1) that owns the interval related with this
+ // event.
+ int ErrorId;
+ };
+ std::priority_queue<Event> Queue;
+ for (const auto &Interval : First) {
+ Queue.push({Interval.first, 1, 0});
+ Queue.push({Interval.second, -1, 0});
+ }
+ for (const auto &Interval : Second) {
+ Queue.push({Interval.first, 1, 1});
+ Queue.push({Interval.second, -1, 1});
+ }
+
+ // Keep track of how many open intervals from each set we have.
+ int Count[2] = {0, 0};
+ // Sit[1][0] will tell if there exists any range that is covered by the
+ // first set but not by the second one, Sit[1][1] will tell if there is a
+ // range covered by both sets, etc.
+ bool Sit[2][2] = {{false, false}, {false, false}};
+
+ while (!Queue.empty()) {
+ unsigned CurrPos = Queue.top().Pos;
+ do {
+ const Event &E = Queue.top();
+ Count[E.ErrorId] += E.Type;
+ Queue.pop();
+ } while (!Queue.empty() && Queue.top().Pos == CurrPos);
+ Sit[Count[0] != 0][Count[1] != 0] = true;
+ }
+
+ // If they never appeared at the same position, there is no overlapping.
+ if (!Sit[1][1])
+ return OK_Disjoint;
+
+ // If both are true, then we have partial overlapping. If both are false,
+ // that means that both interval lists cover exactly the same ranges.
+ if (Sit[0][1] == Sit[1][0])
+ return OK_Overlap;
+
+ // There are points covered by the second list that are not covered by the
+ // first one, but not the other way around.
+ if (Sit[0][1])
+ return OK_FirstInsideSecond;
+
+ return OK_SecondInsideFirst;
+}
+
+void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
+ SmallVectorImpl<ClangTidyError> &Errors) const {
+ // Build the sets of intervals.
+ std::vector<ErrorIntervals> IntervalsList;
+ for (const auto &Error : Errors) {
+ ErrorIntervals Intervals;
+ for (const auto &Replace : Error.Fix) {
+ Intervals.push_back(std::make_pair(
+ Replace.getOffset(), Replace.getOffset() + Replace.getLength()));
+ }
+ IntervalsList.push_back(std::move(Intervals));
+ }
+
+ // If an error overlaps with another one, we don't want to apply any of them,
+ // with an exception: if an error is completely contained inside other error,
+ // we are still allowed to apply the biggest one.
+ int NumErrors = Errors.size();
+ std::vector<bool> Apply(NumErrors, true);
+ for (int I = 0; I < NumErrors; ++I) {
+ for (int J = I + 1; J < NumErrors; ++J) {
+ OverlappingKind Kind =
+ getOverlappingKind(IntervalsList[I], IntervalsList[J]);
+ if (Kind == OK_Overlap || Kind == OK_FirstInsideSecond)
+ Apply[I] = false;
+ if (Kind == OK_Overlap || Kind == OK_SecondInsideFirst)
+ Apply[J] = false;
+ }
+ }
+
+ for (int I = 0; I < NumErrors; ++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();
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
