ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.
In general, the `Explanation` field is optional in `RewriteRule` cases. But,
because the primary purpose of clang-tidy checks is to provide users with
diagnostics, we assume that a missing explanation is a bug. This change adds an
assertion that checks all cases for an explanation, and updates the code to rely
on that assertion correspondingly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D62340
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
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
@@ -29,12 +29,14 @@
using tooling::stencil::cat;
StringRef C = "C", T = "T", E = "E";
- return tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
+ RewriteRule Rule = tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
hasThen(stmt().bind(T)),
hasElse(stmt().bind(E))),
change(statement(RewriteRule::RootID),
cat("if(!(", node(C), ")) ", statement(E),
" else ", statement(T))));
+ Rule.Cases[0].Explanation = tooling::text("no explanation");
+ return Rule;
}
class IfInverterCheck : public TransformerClangTidyCheck {
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -29,12 +29,17 @@
// MyCheck(StringRef Name, ClangTidyContext *Context)
// : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
// };
+//
class TransformerClangTidyCheck : public ClangTidyCheck {
public:
+ // All cases in \p R must have a non-null \c Explanation, even though \c
+ // Explanation is optional for RewriteRule in general. Because the primary
+ // purpose of clang-tidy checks is to provide users with diagnostics, we
+ // assume that a missing explanation is a bug. If no explanation is desired,
+ // indicate that explicitly (for example, `MyRule.Case[0].Explanation =
+ // text("no explanation")`).
TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
- ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
-
+ ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
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
@@ -13,6 +13,17 @@
namespace utils {
using tooling::RewriteRule;
+TransformerClangTidyCheck::TransformerClangTidyCheck(tooling::RewriteRule R,
+ StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), Rule(std::move(R)) {
+ for (const auto &Case : Rule.Cases) {
+ assert(Case.Explanation != nullptr &&
+ "clang-tidy checks must have an explanation by default;"
+ " explicitly provide an empty explanation if none is desired");
+ }
+}
+
void TransformerClangTidyCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
@@ -44,15 +55,13 @@
if (Transformations->empty())
return;
- StringRef Message = "no explanation";
- if (Case.Explanation) {
- if (Expected<std::string> E = Case.Explanation(Result))
- Message = *E;
- else
- llvm::errs() << "Error in explanation: " << llvm::toString(E.takeError())
- << "\n";
+ Expected<std::string> Explanation = Case.Explanation(Result);
+ if (!Explanation) {
+ llvm::errs() << "Error in explanation: "
+ << llvm::toString(Explanation.takeError()) << "\n";
+ return;
}
- DiagnosticBuilder Diag = diag(RootLoc, Message);
+ DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
for (const auto &T : *Transformations) {
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
}
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
@@ -29,12 +29,14 @@
using tooling::stencil::cat;
StringRef C = "C", T = "T", E = "E";
- return tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
+ RewriteRule Rule = tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
hasThen(stmt().bind(T)),
hasElse(stmt().bind(E))),
change(statement(RewriteRule::RootID),
cat("if(!(", node(C), ")) ", statement(E),
" else ", statement(T))));
+ Rule.Cases[0].Explanation = tooling::text("no explanation");
+ return Rule;
}
class IfInverterCheck : public TransformerClangTidyCheck {
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -29,12 +29,17 @@
// MyCheck(StringRef Name, ClangTidyContext *Context)
// : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
// };
+//
class TransformerClangTidyCheck : public ClangTidyCheck {
public:
+ // All cases in \p R must have a non-null \c Explanation, even though \c
+ // Explanation is optional for RewriteRule in general. Because the primary
+ // purpose of clang-tidy checks is to provide users with diagnostics, we
+ // assume that a missing explanation is a bug. If no explanation is desired,
+ // indicate that explicitly (for example, `MyRule.Case[0].Explanation =
+ // text("no explanation")`).
TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
- ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
-
+ ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
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
@@ -13,6 +13,17 @@
namespace utils {
using tooling::RewriteRule;
+TransformerClangTidyCheck::TransformerClangTidyCheck(tooling::RewriteRule R,
+ StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), Rule(std::move(R)) {
+ for (const auto &Case : Rule.Cases) {
+ assert(Case.Explanation != nullptr &&
+ "clang-tidy checks must have an explanation by default;"
+ " explicitly provide an empty explanation if none is desired");
+ }
+}
+
void TransformerClangTidyCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
@@ -44,15 +55,13 @@
if (Transformations->empty())
return;
- StringRef Message = "no explanation";
- if (Case.Explanation) {
- if (Expected<std::string> E = Case.Explanation(Result))
- Message = *E;
- else
- llvm::errs() << "Error in explanation: " << llvm::toString(E.takeError())
- << "\n";
+ Expected<std::string> Explanation = Case.Explanation(Result);
+ if (!Explanation) {
+ llvm::errs() << "Error in explanation: "
+ << llvm::toString(Explanation.takeError()) << "\n";
+ return;
}
- DiagnosticBuilder Diag = diag(RootLoc, Message);
+ DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
for (const auto &T : *Transformations) {
Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
