[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL369914: [clang-tidy] TransformerClangTidyCheck: change
choice of location for… (authored by ymandel, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66676?vs=217146=217161#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66676/new/
https://reviews.llvm.org/D66676
Files:
clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
Index: clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -71,14 +71,6 @@
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- // Verify the existence and validity of the AST node that roots this rule.
- const ast_matchers::BoundNodes::IDToNodeMap = Result.Nodes.getMap();
- auto Root = NodesMap.find(RewriteRule::RootID);
- assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
- SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
- Root->second.getSourceRange().getBegin());
- assert(RootLoc.isValid() && "Invalid location for Root node of match.");
-
assert(Rule && "check() should not fire if Rule is None");
RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, *Rule);
Expected> Transformations =
@@ -99,10 +91,12 @@
<< llvm::toString(Explanation.takeError()) << "\n";
return;
}
- DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
- for (const auto : *Transformations) {
+
+ // Associate the diagnostic with the location of the first change.
+ DiagnosticBuilder Diag =
+ diag((*Transformations)[0].Range.getBegin(), *Explanation);
+ for (const auto : *Transformations)
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
- }
for (const auto : Case.AddedIncludes) {
auto = I.first;
Index: clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -18,18 +18,18 @@
namespace tidy {
namespace utils {
namespace {
+using namespace ::clang::ast_matchers;
+
using tooling::change;
using tooling::IncludeFormat;
+using tooling::node;
using tooling::RewriteRule;
+using tooling::statement;
using tooling::text;
using tooling::stencil::cat;
// Invert the code of an if-statement, while maintaining its semantics.
RewriteRule invertIf() {
- using namespace ::clang::ast_matchers;
- using tooling::node;
- using tooling::statement;
-
StringRef C = "C", T = "T", E = "E";
RewriteRule Rule = tooling::makeRule(
ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
@@ -67,6 +67,56 @@
EXPECT_EQ(Expected, test::runCheckOnCode(Input));
}
+class IntLitCheck : public TransformerClangTidyCheck {
+public:
+ IntLitCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
+change(text("LIT")),
+text("no message")),
+ Name, Context) {}
+};
+
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST(TransformerClangTidyCheckTest, TwoChangesInOneMacroExpansion) {
+ const std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(LIT, LIT); }
+ )cc";
+
+ EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+
+class BinOpCheck : public TransformerClangTidyCheck {
+public:
+ BinOpCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(
+tooling::makeRule(
+binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
+change(node("r"), text("RIGHT")), text("no message")),
+Name, Context) {}
+};
+
+// Tests case where the rule's match spans both source from the macro and its
+// argument, while the change spans only the argument AND there are two such
+// matches. We verify that both replacements succeed.
+TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
+ const std::string Input = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int f() { return M(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int
[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I see, it all makes sense now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66676/new/ https://reviews.llvm.org/D66676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
ymandel marked an inline comment as done. ymandel added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:106 +// argument, while the change spans only the argument AND there are two such +// matches. Here, we expect a conflict between the two matches and the second +// to be ignored. gribozavr wrote: > Sorry, I don't quite understand the comment -- the test has two replacements > (both arguments to the macro are replaced), but the comment says that we only > expect one. Right, that comment needs to be updated. It's leftover from while the fix wasn't done yet... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66676/new/ https://reviews.llvm.org/D66676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
ymandel updated this revision to Diff 217146.
ymandel added a comment.
fixed comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66676/new/
https://reviews.llvm.org/D66676
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -18,18 +18,18 @@
namespace tidy {
namespace utils {
namespace {
+using namespace ::clang::ast_matchers;
+
using tooling::change;
using tooling::IncludeFormat;
+using tooling::node;
using tooling::RewriteRule;
+using tooling::statement;
using tooling::text;
using tooling::stencil::cat;
// Invert the code of an if-statement, while maintaining its semantics.
RewriteRule invertIf() {
- using namespace ::clang::ast_matchers;
- using tooling::node;
- using tooling::statement;
-
StringRef C = "C", T = "T", E = "E";
RewriteRule Rule = tooling::makeRule(
ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
@@ -67,6 +67,56 @@
EXPECT_EQ(Expected, test::runCheckOnCode(Input));
}
+class IntLitCheck : public TransformerClangTidyCheck {
+public:
+ IntLitCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
+change(text("LIT")),
+text("no message")),
+ Name, Context) {}
+};
+
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST(TransformerClangTidyCheckTest, TwoChangesInOneMacroExpansion) {
+ const std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(LIT, LIT); }
+ )cc";
+
+ EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+
+class BinOpCheck : public TransformerClangTidyCheck {
+public:
+ BinOpCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(
+tooling::makeRule(
+binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
+change(node("r"), text("RIGHT")), text("no message")),
+Name, Context) {}
+};
+
+// Tests case where the rule's match spans both source from the macro and its
+// argument, while the change spans only the argument AND there are two such
+// matches. We verify that both replacements succeed.
+TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
+ const std::string Input = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int f() { return M(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int f() { return M(RIGHT, RIGHT); }
+ )cc";
+
+ EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+
// A trivial rewrite-rule generator that requires Objective-C code.
Optional needsObjC(const LangOptions ,
const ClangTidyCheck::OptionsView ) {
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -71,14 +71,6 @@
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- // Verify the existence and validity of the AST node that roots this rule.
- const ast_matchers::BoundNodes::IDToNodeMap = Result.Nodes.getMap();
- auto Root = NodesMap.find(RewriteRule::RootID);
- assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
- SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
- Root->second.getSourceRange().getBegin());
- assert(RootLoc.isValid() && "Invalid location for Root node of match.");
-
assert(Rule && "check() should not fire if Rule is None");
RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, *Rule);
Expected> Transformations =
@@ -99,10 +91,12 @@
<< llvm::toString(Explanation.takeError()) << "\n";
return;
}
- DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
- for (const auto : *Transformations) {
+
+ // Associate the diagnostic with the location of the first change.
+ DiagnosticBuilder Diag =
+ diag((*Transformations)[0].Range.getBegin(), *Explanation);
+ for (const auto : *Transformations)
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
- }
for (const auto : Case.AddedIncludes) {
auto = I.first;
[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
gribozavr added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:106 +// argument, while the change spans only the argument AND there are two such +// matches. Here, we expect a conflict between the two matches and the second +// to be ignored. Sorry, I don't quite understand the comment -- the test has two replacements (both arguments to the macro are replaced), but the comment says that we only expect one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66676/new/ https://reviews.llvm.org/D66676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.
ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.
This patch changes the location specified to the
`ClangTidyCheck::diag()`. Currently, the beginning of the matched range is
used. This patch uses the beginning of the first fix's range. This change both
simplifies the code and (hopefully) gives a more intuitive result: the reported
location aligns with the fix(es) provided, rather than the (arbitrary) range of
the rule's match.
N.B. this patch will break the line offset numbers in lit tests if the first fix
is not at the beginning of the match.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66676
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -18,18 +18,18 @@
namespace tidy {
namespace utils {
namespace {
+using namespace ::clang::ast_matchers;
+
using tooling::change;
using tooling::IncludeFormat;
+using tooling::node;
using tooling::RewriteRule;
+using tooling::statement;
using tooling::text;
using tooling::stencil::cat;
// Invert the code of an if-statement, while maintaining its semantics.
RewriteRule invertIf() {
- using namespace ::clang::ast_matchers;
- using tooling::node;
- using tooling::statement;
-
StringRef C = "C", T = "T", E = "E";
RewriteRule Rule = tooling::makeRule(
ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
@@ -67,6 +67,57 @@
EXPECT_EQ(Expected, test::runCheckOnCode(Input));
}
+class IntLitCheck : public TransformerClangTidyCheck {
+public:
+ IntLitCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
+change(text("LIT")),
+text("no message")),
+ Name, Context) {}
+};
+
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST(TransformerClangTidyCheckTest, TwoChangesInOneMacroExpansion) {
+ const std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+int f() { return PLUS(LIT, LIT); }
+ )cc";
+
+ EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+
+class BinOpCheck : public TransformerClangTidyCheck {
+public:
+ BinOpCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(
+tooling::makeRule(
+binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
+change(node("r"), text("RIGHT")), text("no message")),
+Name, Context) {}
+};
+
+// Tests case where the rule's match spans both source from the macro and its
+// argument, while the change spans only the argument AND there are two such
+// matches. Here, we expect a conflict between the two matches and the second
+// to be ignored.
+TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
+ const std::string Input = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int f() { return M(3, 4); }
+ )cc";
+ const std::string Expected = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+int f() { return M(RIGHT, RIGHT); }
+ )cc";
+
+ EXPECT_EQ(Expected, test::runCheckOnCode(Input));
+}
+
// A trivial rewrite-rule generator that requires Objective-C code.
Optional needsObjC(const LangOptions ,
const ClangTidyCheck::OptionsView ) {
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -71,14 +71,6 @@
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- // Verify the existence and validity of the AST node that roots this rule.
- const ast_matchers::BoundNodes::IDToNodeMap = Result.Nodes.getMap();
- auto Root = NodesMap.find(RewriteRule::RootID);
- assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
- SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
- Root->second.getSourceRange().getBegin());
- assert(RootLoc.isValid() && "Invalid location for Root node of match.");
-
assert(Rule && "check() should not fire if Rule is None");
RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, *Rule);
Expected> Transformations =
@@ -99,10 +91,12 @@
<<
