[clang-tools-extra] 068da2c - [clang-tidy] Allow `TransformerClangTidyCheck` clients to set the rule directly.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 18 10:27:19 PST 2020
Author: Yitzhak Mandelbaum
Date: 2020-11-18T18:25:21Z
New Revision: 068da2c749a58b46bd59381890a6a137d6e3128e
URL: https://github.com/llvm/llvm-project/commit/068da2c749a58b46bd59381890a6a137d6e3128e
DIFF: https://github.com/llvm/llvm-project/commit/068da2c749a58b46bd59381890a6a137d6e3128e.diff
LOG: [clang-tidy] Allow `TransformerClangTidyCheck` clients to set the rule directly.
Adds support for setting the `Rule` field. In the process, refactors the code that accesses that field and adds a constructor that doesn't require a rule argument.
This feature is needed by checks that must set the rule *after* the check class
is constructed. For example, any check that maintains state to be accessed from
the rule needs this support. Since the object's fields are not initialized when
the superclass constructor is called, they can't be (safely) captured by a rule
passed to the existing constructor. This patch allows constructing the check
superclass fully before setting the rule.
As a driveby fix, removed the "optional" from the rule, since rules are just a
set of cases, so empty rules are evident.
Differential Revision: https://reviews.llvm.org/D91544
Added:
Modified:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index ae2ae86f4665..e93f3fbf21d7 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -21,6 +21,18 @@ static bool hasExplanation(const RewriteRule::Case &C) {
}
#endif
+static void verifyRule(const RewriteRule &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(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ Inserter(
+ Options.getLocalOrGlobal("IncludeStyle", IncludeSorter::IS_LLVM)) {}
+
// 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,
@@ -31,24 +43,21 @@ TransformerClangTidyCheck::TransformerClangTidyCheck(
const OptionsView &)>
MakeRule,
StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(MakeRule(getLangOpts(), Options)),
- Inserter(
- Options.getLocalOrGlobal("IncludeStyle", 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(Name, Context) {
+ if (Optional<RewriteRule> R = MakeRule(getLangOpts(), Options))
+ setRule(std::move(*R));
}
TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,
StringRef Name,
ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context), Rule(std::move(R)),
- Inserter(
- Options.getLocalOrGlobal("IncludeStyle", 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");
+ : TransformerClangTidyCheck(Name, Context) {
+ setRule(std::move(R));
+}
+
+void TransformerClangTidyCheck::setRule(transformer::RewriteRule R) {
+ verifyRule(R);
+ Rule = std::move(R);
}
void TransformerClangTidyCheck::registerPPCallbacks(
@@ -58,8 +67,8 @@ void TransformerClangTidyCheck::registerPPCallbacks(
void TransformerClangTidyCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
- if (Rule)
- for (auto &Matcher : transformer::detail::buildMatchers(*Rule))
+ if (!Rule.Cases.empty())
+ for (auto &Matcher : transformer::detail::buildMatchers(Rule))
Finder->addDynamicMatcher(Matcher, this);
}
@@ -68,8 +77,7 @@ void TransformerClangTidyCheck::check(
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- assert(Rule && "check() should not fire if Rule is None");
- RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, *Rule);
+ RewriteRule::Case Case = transformer::detail::findSelectedCase(Result, Rule);
Expected<SmallVector<transformer::Edit, 1>> Edits = Case.Edits(Result);
if (!Edits) {
llvm::errs() << "Rewrite failed: " << llvm::toString(Edits.takeError())
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
index 404f474a24ca..9736e64e7c31 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -19,7 +19,7 @@ namespace clang {
namespace tidy {
namespace utils {
-// A base class for defining a ClangTidy check based on a `RewriteRule`.
+/// A base class for defining a ClangTidy check based on a `RewriteRule`.
//
// For example, given a rule `MyCheckAsRewriteRule`, one can define a tidy check
// as follows:
@@ -38,24 +38,23 @@ namespace utils {
// 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(StringRef Name, ClangTidyContext *Context);
+
+ /// DEPRECATED: prefer the two argument constructor in conjunction with
+ /// \c setRule.
+ ///
+ /// \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.
+ ///
+ /// See \c setRule for constraints on the rule.
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.
+ /// Convenience overload of the constructor when the rule doesn't have any
+ /// dependies.
TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
ClangTidyContext *Context);
@@ -68,8 +67,17 @@ class TransformerClangTidyCheck : public ClangTidyCheck {
/// the overridden method.
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ /// Set the rule that this check implements. 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).
+ void setRule(transformer::RewriteRule R);
+
private:
- Optional<transformer::RewriteRule> Rule;
+ transformer::RewriteRule Rule;
IncludeInserter Inserter;
};
More information about the cfe-commits
mailing list