njames93 updated this revision to Diff 267161.
njames93 marked 3 inline comments as done.
njames93 added a comment.
- Add back old constructors, one being deprecated
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80697/new/
https://reviews.llvm.org/D80697
Files:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
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> 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<RewriteRule> 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<RewriteRule> 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<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> 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<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> 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 <IncludeFormat Format>
class IncludeCheck : public TransformerClangTidyCheck {
public:
IncludeCheck(StringRef Name, ClangTidyContext *Context)
- : TransformerClangTidyCheck(replaceCall(Format), Name, Context) {}
+ : TransformerClangTidyCheck(Name, Context) {}
+
+ Optional<RewriteRule> 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<RewriteRule> 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) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
-----------------------------
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
@@ -9,14 +9,14 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
-#include "../ClangTidy.h"
+#include "../ClangTidyCheck.h"
#include "IncludeInserter.h"
#include "IncludeSorter.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Frontend/CompilerInstance.h"
#include "clang/Tooling/Transformer/Transformer.h"
-#include <deque>
-#include <vector>
+#include "llvm/ADT/Optional.h"
+
+#define DEP_TRANSFORMER_CTOR \
+ [[deprecated("Please use buildRule() to define the RewriteRule")]]
namespace clang {
namespace tidy {
@@ -30,7 +30,11 @@
// class MyCheck : public TransformerClangTidyCheck {
// public:
// MyCheck(StringRef Name, ClangTidyContext *Context)
-// : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
+// : TransformerClangTidyCheck(Name, Context) {}
+//
+// Optional<RewriteRule> buildRule() const override {
+// return MyCheckAsRewriteRule;
+// }
// };
//
// `TransformerClangTidyCheck` recognizes this clang-tidy option:
@@ -41,29 +45,17 @@
// 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(StringRef Name, ClangTidyContext *Context);
TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
ClangTidyContext *Context);
+ DEP_TRANSFORMER_CTOR TransformerClangTidyCheck(
+ std::function<Optional<transformer::RewriteRule>(const LangOptions &,
+ const OptionsView &)>
+ MakeRule,
+ 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 +63,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> buildRule() const { return None; }
+
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,17 +20,17 @@
}
#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)),
+TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
+ IncludeSorter::getMapping(),
+ IncludeSorter::IS_LLVM)) {}
+
+TransformerClangTidyCheck::TransformerClangTidyCheck(transformer::RewriteRule R,
+ StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), Rule(std::move(R)), HasMadeRule(true),
IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
IncludeSorter::getMapping(),
IncludeSorter::IS_LLVM)) {
@@ -40,20 +40,25 @@
" explicitly provide an empty explanation if none is desired");
}
-TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,
- StringRef Name,
- ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(std::move(R)),
+TransformerClangTidyCheck::TransformerClangTidyCheck(
+ std::function<Optional<transformer::RewriteRule>(const LangOptions &,
+ const OptionsView &)>
+ MakeRule,
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), HasMadeRule(true),
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");
+ Rule = MakeRule(getLangOpts(), Options);
+ 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");
}
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 +72,7 @@
void TransformerClangTidyCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
+ instantiateRule();
if (Rule)
for (auto &Matcher : transformer::detail::buildMatchers(*Rule))
Finder->addDynamicMatcher(Matcher, this);
@@ -118,6 +124,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
Index: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
+++ 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<transformer::RewriteRule> buildRule() const override;
+
private:
- const std::vector<std::string> StringLikeClassesOption;
- const std::string AbseilStringsMatchHeaderOption;
+ const std::vector<std::string> StringLikeClasses;
+ const std::string AbseilStringsMatchHeader;
};
} // namespace abseil
Index: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
+++ 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<transformer::RewriteRule>
-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<std::string> StringLikeClassNames =
- utils::options::parseStringList(
- Options.get("StringLikeClasses", DefaultStringLikeClasses));
- const std::string AbseilStringsMatchHeader =
- Options.get("AbseilStringsMatchHeader", DefaultAbseilStringsMatchHeader);
+llvm::Optional<transformer::RewriteRule>
+StringFindStrContainsCheck::buildRule() const {
auto StringLikeClass = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
- 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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
