njames93 created this revision.
njames93 added reviewers: aaron.ballman, gribozavr2, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
Changed `TransformerClangTidyCheck` to have a virtual method generate the rule.
This has the advantages of making handling of any check options much simpler.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80697
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
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
@@ -10,6 +10,7 @@
#include "ClangTidyTest.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Tooling/Transformer/RangeSelector.h"
+#include "clang/Tooling/Transformer/RewriteRule.h"
#include "clang/Tooling/Transformer/Stencil.h"
#include "clang/Tooling/Transformer/Transformer.h"
#include "gtest/gtest.h"
@@ -28,23 +29,23 @@
using transformer::statement;
// Invert the code of an if-statement, while maintaining its semantics.
-RewriteRule invertIf() {
- StringRef C = "C", T = "T", E = "E";
- RewriteRule Rule = tooling::makeRule(
- ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
- hasElse(stmt().bind(E))),
- change(statement(std::string(RewriteRule::RootID)),
- cat("if(!(", node(std::string(C)), ")) ",
- statement(std::string(E)), " else ",
- statement(std::string(T)))),
- cat("negate condition and reverse `then` and `else` branches"));
- return Rule;
-}
-
class IfInverterCheck : public TransformerClangTidyCheck {
public:
IfInverterCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(invertIf(), Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> makeRule() const override {
+ StringRef C = "C", T = "T", E = "E";
+ RewriteRule Rule = tooling::makeRule(
+ ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+ hasElse(stmt().bind(E))),
+ change(statement(std::string(RewriteRule::RootID)),
+ cat("if(!(", node(std::string(C)), ")) ",
+ statement(std::string(E)), " else ",
+ statement(std::string(T)))),
+ cat("negate condition and reverse `then` and `else` branches"));
+ return Rule;
+ }
};
// Basic test of using a rewrite rule as a ClangTidy.
@@ -70,10 +71,12 @@
class IntLitCheck : public TransformerClangTidyCheck {
public:
IntLitCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
- change(cat("LIT")),
- cat("no message")),
- Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> makeRule() const override {
+ return tooling::makeRule(integerLiteral(), change(cat("LIT")),
+ cat("no message"));
+ }
};
// Tests that two changes in a single macro expansion do not lead to conflicts
@@ -94,11 +97,13 @@
class BinOpCheck : public TransformerClangTidyCheck {
public:
BinOpCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(
- tooling::makeRule(
- binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
- change(node("r"), cat("RIGHT")), cat("no message")),
- Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> makeRule() const override {
+ return tooling::makeRule(
+ binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
+ change(node("r"), cat("RIGHT")), cat("no message"));
+ }
};
// Tests case where the rule's match spans both source from the macro and its
@@ -118,18 +123,20 @@
}
// A trivial rewrite-rule generator that requires Objective-C code.
-Optional<RewriteRule> needsObjC(const LangOptions &LangOpts,
- const ClangTidyCheck::OptionsView &Options) {
- if (!LangOpts.ObjC)
- return None;
- return tooling::makeRule(clang::ast_matchers::functionDecl(),
- change(cat("void changed() {}")), cat("no message"));
-}
-
class NeedsObjCCheck : public TransformerClangTidyCheck {
public:
NeedsObjCCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(needsObjC, Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.ObjC;
+ }
+
+ Optional<RewriteRule> makeRule() const override {
+ return tooling::makeRule(clang::ast_matchers::functionDecl(),
+ change(cat("void changed() {}")),
+ cat("no message"));
+ }
};
// Verify that the check only rewrites the code when the input is Objective-C.
@@ -143,18 +150,21 @@
}
// A trivial rewrite rule generator that checks config options.
-Optional<RewriteRule> noSkip(const LangOptions &LangOpts,
- const ClangTidyCheck::OptionsView &Options) {
- if (Options.get("Skip", "false") == "true")
- return None;
- return tooling::makeRule(clang::ast_matchers::functionDecl(),
- change(cat("void nothing()")), cat("no message"));
-}
-
class ConfigurableCheck : public TransformerClangTidyCheck {
public:
ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(noSkip, Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context),
+ Skip(Options.get("Skip", false)) {}
+
+ Optional<RewriteRule> makeRule() const override {
+ if (Skip)
+ return None;
+ return tooling::makeRule(clang::ast_matchers::functionDecl(),
+ change(cat("void nothing()")), cat("no message"));
+ }
+
+private:
+ const bool Skip;
};
// Tests operation with config option "Skip" set to true and false.
@@ -172,20 +182,20 @@
Input, nullptr, "input.cc", None, Options));
}
-RewriteRule replaceCall(IncludeFormat Format) {
- using namespace ::clang::ast_matchers;
- RewriteRule Rule =
- tooling::makeRule(callExpr(callee(functionDecl(hasName("f")))),
- change(cat("other()")), cat("no message"));
- addInclude(Rule, "clang/OtherLib.h", Format);
- return Rule;
-}
-
template <IncludeFormat Format>
class IncludeCheck : public TransformerClangTidyCheck {
public:
IncludeCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(replaceCall(Format), Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> makeRule() const override {
+ using namespace ::clang::ast_matchers;
+ RewriteRule Rule =
+ tooling::makeRule(callExpr(callee(functionDecl(hasName("f")))),
+ change(cat("other()")), cat("no message"));
+ addInclude(Rule, "clang/OtherLib.h", Format);
+ return Rule;
+ }
};
TEST(TransformerClangTidyCheckTest, AddIncludeQuoted) {
@@ -222,17 +232,17 @@
}
class IncludeOrderCheck : public TransformerClangTidyCheck {
- static RewriteRule rule() {
+public:
+ IncludeOrderCheck(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> makeRule() const override {
using namespace ::clang::ast_matchers;
RewriteRule Rule = transformer::makeRule(integerLiteral(), change(cat("5")),
cat("no message"));
addInclude(Rule, "bar.h", IncludeFormat::Quoted);
return Rule;
}
-
-public:
- IncludeOrderCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(rule(), Name, Context) {}
};
TEST(TransformerClangTidyCheckTest, AddIncludeObeysSortStyleLocalOption) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -209,6 +209,10 @@
- For 'run-clang-tidy.py' add option to use alpha checkers from clang-analyzer.
+- ``TransformerClangTidyChecks`` have been reworked to have enable simpler
+ handling of options. This change will require a small tweak to any check that
+ derives from the ``TransformerClangTidyChecks`` base class.
+
Improvements to include-fixer
-----------------------------
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
@@ -30,7 +30,11 @@
// class MyCheck : public TransformerClangTidyCheck {
// public:
// MyCheck(StringRef Name, ClangTidyContext *Context)
-// : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
+// : TransformerClangTidyCheck(Name, Context) {}
+//
+// Optional<RewriteRule> makeRule() const override {
+// return MyCheckAsRewriteRule;
+// }
// };
//
// `TransformerClangTidyCheck` recognizes this clang-tidy option:
@@ -41,29 +45,10 @@
// includes.
class TransformerClangTidyCheck : public ClangTidyCheck {
public:
- // \p MakeRule generates the rewrite rule to be used by the check, based on
- // the given language and clang-tidy options. It can return \c None to handle
- // cases where the options disable the check.
- //
- // All cases in the rule generated by \p MakeRule 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, by
- // passing `text("no explanation")` to `makeRule` as the `Explanation`
- // argument).
- TransformerClangTidyCheck(std::function<Optional<transformer::RewriteRule>(
- const LangOptions &, const OptionsView &)>
- MakeRule,
- StringRef Name, ClangTidyContext *Context);
-
- // Convenience overload of the constructor when the rule doesn't depend on any
- // of the language or clang-tidy options.
- TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
- ClangTidyContext *Context);
+ TransformerClangTidyCheck(StringRef Name, ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
- Preprocessor *ModuleExpanderPP) override;
+ Preprocessor *ModuleExpanderPP) final;
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
@@ -71,8 +56,21 @@
/// the overridden method.
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ /// This generates the rewrite rule to be used by the check.It can return \c
+ /// None to handle cases where the options disable the check.
+ ///
+ /// All cases in the rule 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, by passing `text("no explanation")`
+ /// to `makeRule` as the `Explanation` argument).
+ virtual Optional<transformer::RewriteRule> makeRule() const = 0;
+
private:
+ void instantiateRule();
Optional<transformer::RewriteRule> Rule;
+ bool HasMadeRule = false;
const IncludeSorter::IncludeStyle IncludeStyle;
std::unique_ptr<IncludeInserter> Inserter;
};
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
@@ -20,40 +20,16 @@
}
#endif
-// This constructor cannot dispatch to the simpler one (below), because, in
-// order to get meaningful results from `getLangOpts` and `Options`, we need the
-// `ClangTidyCheck()` constructor to have been called. If we were to dispatch,
-// we would be accessing `getLangOpts` and `Options` before the underlying
-// `ClangTidyCheck` instance was properly initialized.
-TransformerClangTidyCheck::TransformerClangTidyCheck(
- std::function<Optional<RewriteRule>(const LangOptions &,
- const OptionsView &)>
- MakeRule,
- StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(MakeRule(getLangOpts(), Options)),
- IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
- IncludeSorter::getMapping(),
- IncludeSorter::IS_LLVM)) {
- if (Rule)
- assert(llvm::all_of(Rule->Cases, hasExplanation) &&
- "clang-tidy checks must have an explanation by default;"
- " explicitly provide an empty explanation if none is desired");
-}
-
-TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,
- StringRef Name,
+TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(std::move(R)),
+ : ClangTidyCheck(Name, Context),
IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
IncludeSorter::getMapping(),
- IncludeSorter::IS_LLVM)) {
- assert(llvm::all_of(Rule->Cases, hasExplanation) &&
- "clang-tidy checks must have an explanation by default;"
- " explicitly provide an empty explanation if none is desired");
-}
+ IncludeSorter::IS_LLVM)) {}
void TransformerClangTidyCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+ instantiateRule();
// Only allocate and register the IncludeInsert when some `Case` will add
// includes.
if (Rule && llvm::any_of(Rule->Cases, [](const RewriteRule::Case &C) {
@@ -67,6 +43,7 @@
void TransformerClangTidyCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
+ instantiateRule();
if (Rule)
for (auto &Matcher : transformer::detail::buildMatchers(*Rule))
Finder->addDynamicMatcher(Matcher, this);
@@ -118,6 +95,16 @@
IncludeSorter::getMapping());
}
+void TransformerClangTidyCheck::instantiateRule() {
+ if (!HasMadeRule) {
+ HasMadeRule = true;
+ if ((Rule = makeRule()))
+ assert(llvm::all_of(Rule->Cases, hasExplanation) &&
+ "clang-tidy checks must have an explanation by default;"
+ " explicitly provide an empty explanation if none is desired");
+ }
+}
+
} // namespace utils
} // namespace tidy
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
