diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h --- a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h +++ b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h @@ -27,9 +27,11 @@ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + Optional buildRule() const override; + private: - const std::vector StringLikeClassesOption; - const std::string AbseilStringsMatchHeaderOption; + const std::vector StringLikeClasses; + const std::string AbseilStringsMatchHeader; }; } // namespace abseil diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp --- a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp @@ -32,24 +32,11 @@ "::absl::string_view"; static const char DefaultAbseilStringsMatchHeader[] = "absl/strings/match.h"; -static llvm::Optional -MakeRule(const LangOptions &LangOpts, - const ClangTidyCheck::OptionsView &Options) { - // Parse options. - // - // FIXME(tdl-g): These options are being parsed redundantly with the - // constructor because TransformerClangTidyCheck forces us to provide MakeRule - // before "this" is fully constructed, but StoreOptions requires us to store - // the parsed options in "this". We need to fix TransformerClangTidyCheck and - // then we can clean this up. - const std::vector StringLikeClassNames = - utils::options::parseStringList( - Options.get("StringLikeClasses", DefaultStringLikeClasses)); - const std::string AbseilStringsMatchHeader = - Options.get("AbseilStringsMatchHeader", DefaultAbseilStringsMatchHeader); +llvm::Optional +StringFindStrContainsCheck::buildRule() const { auto StringLikeClass = cxxRecordDecl(hasAnyName(SmallVector( - StringLikeClassNames.begin(), StringLikeClassNames.end()))); + StringLikeClasses.begin(), StringLikeClasses.end()))); auto StringType = hasUnqualifiedDesugaredType(recordType(hasDeclaration(StringLikeClass))); auto CharStarType = @@ -85,11 +72,11 @@ StringFindStrContainsCheck::StringFindStrContainsCheck( StringRef Name, ClangTidyContext *Context) - : TransformerClangTidyCheck(&MakeRule, Name, Context), - StringLikeClassesOption(utils::options::parseStringList( + : TransformerClangTidyCheck(Name, Context), + StringLikeClasses(utils::options::parseStringList( Options.get("StringLikeClasses", DefaultStringLikeClasses))), - AbseilStringsMatchHeaderOption(Options.get( - "AbseilStringsMatchHeader", DefaultAbseilStringsMatchHeader)) {} + AbseilStringsMatchHeader(Options.get("AbseilStringsMatchHeader", + DefaultAbseilStringsMatchHeader)) {} bool StringFindStrContainsCheck::isLanguageVersionSupported( const LangOptions &LangOpts) const { @@ -100,9 +87,8 @@ ClangTidyOptions::OptionMap &Opts) { TransformerClangTidyCheck::storeOptions(Opts); Options.store(Opts, "StringLikeClasses", - utils::options::serializeStringList(StringLikeClassesOption)); - Options.store(Opts, "AbseilStringsMatchHeader", - AbseilStringsMatchHeaderOption); + utils::options::serializeStringList(StringLikeClasses)); + Options.store(Opts, "AbseilStringsMatchHeader", AbseilStringsMatchHeader); } } // namespace abseil diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h +++ b/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 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( - 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 buildRule() const = 0; + private: + void instantiateRule(); Optional Rule; + bool HasMadeRule = false; const IncludeSorter::IncludeStyle IncludeStyle; std::unique_ptr Inserter; }; diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp --- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp +++ b/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(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 = buildRule())) + 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -216,6 +216,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 ----------------------------- diff --git a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp --- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp +++ b/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 buildRule() 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 buildRule() 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 buildRule() 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 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 buildRule() 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 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 buildRule() 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 class IncludeCheck : public TransformerClangTidyCheck { public: IncludeCheck(StringRef Name, ClangTidyContext *Context) - : TransformerClangTidyCheck(replaceCall(Format), Name, Context) {} + : TransformerClangTidyCheck(Name, Context) {} + + Optional buildRule() 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 buildRule() 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) {