[clang] 8351726 - Revert "[libTooling] Generalize string explanation as templated metadata"
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 21 12:07:11 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-03-21T19:06:59Z
New Revision: 8351726e6dba0ca08b477276f22361c2ab609ecf
URL: https://github.com/llvm/llvm-project/commit/8351726e6dba0ca08b477276f22361c2ab609ecf
DIFF: https://github.com/llvm/llvm-project/commit/8351726e6dba0ca08b477276f22361c2ab609ecf.diff
LOG: Revert "[libTooling] Generalize string explanation as templated metadata"
This reverts commit 18440547d3520b78c9ab929685309419fc1fbe95. Causing failures
in some build modes.
e.g. https://lab.llvm.org/buildbot/#/builders/217/builds/1886
Added:
Modified:
clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/include/clang/Tooling/Transformer/Transformer.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/lib/Tooling/Transformer/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp
index e6617fb7dd89e..bbf860342fee6 100644
--- a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp
@@ -23,7 +23,7 @@ namespace clang {
namespace tidy {
namespace abseil {
-RewriteRuleWith<std::string> CleanupCtadCheckImpl() {
+RewriteRule CleanupCtadCheckImpl() {
auto warning_message = cat("prefer absl::Cleanup's class template argument "
"deduction pattern in C++17 and higher");
diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
index 964826a3bd615..601b987d332b6 100644
--- a/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
@@ -30,7 +30,7 @@ using ::clang::transformer::cat;
using ::clang::transformer::changeTo;
using ::clang::transformer::makeRule;
using ::clang::transformer::node;
-using ::clang::transformer::RewriteRuleWith;
+using ::clang::transformer::RewriteRule;
AST_MATCHER(Type, isCharType) { return Node.isCharType(); }
@@ -39,7 +39,7 @@ static const char DefaultStringLikeClasses[] = "::std::basic_string;"
"::absl::string_view";
static const char DefaultAbseilStringsMatchHeader[] = "absl/strings/match.h";
-static transformer::RewriteRuleWith<std::string>
+static transformer::RewriteRule
makeRewriteRule(const std::vector<std::string> &StringLikeClassNames,
StringRef AbseilStringsMatchHeader) {
auto StringLikeClass = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
@@ -62,7 +62,7 @@ makeRewriteRule(const std::vector<std::string> &StringLikeClassNames,
hasArgument(1, cxxDefaultArgExpr())),
onImplicitObjectArgument(expr().bind("string_being_searched")));
- RewriteRuleWith<std::string> Rule = applyFirst(
+ RewriteRule Rule = applyFirst(
{makeRule(
binaryOperator(hasOperatorName("=="),
hasOperands(ignoringParenImpCasts(StringNpos),
diff --git a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
index 205f0c4ff9c6f..b45aa93533b08 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
@@ -37,7 +37,7 @@ AST_MATCHER(clang::VarDecl, isDirectInitialization) {
} // namespace
-RewriteRuleWith<std::string> StringviewNullptrCheckImpl() {
+RewriteRule StringviewNullptrCheckImpl() {
auto construction_warning =
cat("constructing basic_string_view from null is undefined; replace with "
"the default constructor");
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index c18838fb0f992..9f64c562600bc 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -13,16 +13,16 @@
namespace clang {
namespace tidy {
namespace utils {
-using transformer::RewriteRuleWith;
+using transformer::RewriteRule;
#ifndef NDEBUG
-static bool hasGenerator(const transformer::Generator<std::string> &G) {
- return G != nullptr;
+static bool hasExplanation(const RewriteRule::Case &C) {
+ return C.Explanation != nullptr;
}
#endif
-static void verifyRule(const RewriteRuleWith<std::string> &Rule) {
- assert(llvm::all_of(Rule.Metadata, hasGenerator) &&
+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");
}
@@ -39,24 +39,23 @@ TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
// we would be accessing `getLangOpts` and `Options` before the underlying
// `ClangTidyCheck` instance was properly initialized.
TransformerClangTidyCheck::TransformerClangTidyCheck(
- std::function<Optional<RewriteRuleWith<std::string>>(const LangOptions &,
- const OptionsView &)>
+ std::function<Optional<RewriteRule>(const LangOptions &,
+ const OptionsView &)>
MakeRule,
StringRef Name, ClangTidyContext *Context)
: TransformerClangTidyCheck(Name, Context) {
- if (Optional<RewriteRuleWith<std::string>> R =
- MakeRule(getLangOpts(), Options))
+ if (Optional<RewriteRule> R = MakeRule(getLangOpts(), Options))
setRule(std::move(*R));
}
-TransformerClangTidyCheck::TransformerClangTidyCheck(
- RewriteRuleWith<std::string> R, StringRef Name, ClangTidyContext *Context)
+TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,
+ StringRef Name,
+ ClangTidyContext *Context)
: TransformerClangTidyCheck(Name, Context) {
setRule(std::move(R));
}
-void TransformerClangTidyCheck::setRule(
- transformer::RewriteRuleWith<std::string> R) {
+void TransformerClangTidyCheck::setRule(transformer::RewriteRule R) {
verifyRule(R);
Rule = std::move(R);
}
@@ -78,9 +77,8 @@ void TransformerClangTidyCheck::check(
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- size_t I = transformer::detail::findSelectedCase(Result, Rule);
- Expected<SmallVector<transformer::Edit, 1>> Edits =
- Rule.Cases[I].Edits(Result);
+ 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())
<< "\n";
@@ -91,7 +89,7 @@ void TransformerClangTidyCheck::check(
if (Edits->empty())
return;
- Expected<std::string> Explanation = Rule.Metadata[I]->eval(Result);
+ Expected<std::string> Explanation = Case.Explanation->eval(Result);
if (!Explanation) {
llvm::errs() << "Error in explanation: "
<< llvm::toString(Explanation.takeError()) << "\n";
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
index 38ca2011fba14..d26737935b1aa 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -48,16 +48,15 @@ class TransformerClangTidyCheck : public ClangTidyCheck {
/// cases where the options disable the check.
///
/// See \c setRule for constraints on the rule.
- TransformerClangTidyCheck(
- std::function<Optional<transformer::RewriteRuleWith<std::string>>(
- const LangOptions &, const OptionsView &)>
- MakeRule,
- StringRef Name, ClangTidyContext *Context);
+ 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 have any
/// dependencies.
- TransformerClangTidyCheck(transformer::RewriteRuleWith<std::string> R,
- StringRef Name, ClangTidyContext *Context);
+ TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
+ ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
@@ -75,10 +74,10 @@ class TransformerClangTidyCheck : public ClangTidyCheck {
/// 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::RewriteRuleWith<std::string> R);
+ void setRule(transformer::RewriteRule R);
private:
- transformer::RewriteRuleWith<std::string> Rule;
+ transformer::RewriteRule Rule;
IncludeInserter Inserter;
};
diff --git a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
index 832b8b86e1266..53ea4f5977587 100644
--- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -28,14 +28,14 @@ using transformer::IncludeFormat;
using transformer::makeRule;
using transformer::node;
using transformer::noopEdit;
-using transformer::RewriteRuleWith;
+using transformer::RewriteRule;
using transformer::RootID;
using transformer::statement;
// Invert the code of an if-statement, while maintaining its semantics.
-RewriteRuleWith<std::string> invertIf() {
+RewriteRule invertIf() {
StringRef C = "C", T = "T", E = "E";
- RewriteRuleWith<std::string> Rule = makeRule(
+ RewriteRule Rule = makeRule(
ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
hasElse(stmt().bind(E))),
change(statement(RootID), cat("if(!(", node(std::string(C)), ")) ",
@@ -140,9 +140,8 @@ TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
}
// A trivial rewrite-rule generator that requires Objective-C code.
-Optional<RewriteRuleWith<std::string>>
-needsObjC(const LangOptions &LangOpts,
- const ClangTidyCheck::OptionsView &Options) {
+Optional<RewriteRule> needsObjC(const LangOptions &LangOpts,
+ const ClangTidyCheck::OptionsView &Options) {
if (!LangOpts.ObjC)
return None;
return makeRule(clang::ast_matchers::functionDecl(),
@@ -166,9 +165,8 @@ TEST(TransformerClangTidyCheckTest, DisableByLang) {
}
// A trivial rewrite rule generator that checks config options.
-Optional<RewriteRuleWith<std::string>>
-noSkip(const LangOptions &LangOpts,
- const ClangTidyCheck::OptionsView &Options) {
+Optional<RewriteRule> noSkip(const LangOptions &LangOpts,
+ const ClangTidyCheck::OptionsView &Options) {
if (Options.get("Skip", "false") == "true")
return None;
return makeRule(clang::ast_matchers::functionDecl(),
@@ -196,11 +194,10 @@ TEST(TransformerClangTidyCheckTest, DisableByConfig) {
Input, nullptr, "input.cc", None, Options));
}
-RewriteRuleWith<std::string> replaceCall(IncludeFormat Format) {
+RewriteRule replaceCall(IncludeFormat Format) {
using namespace ::clang::ast_matchers;
- RewriteRuleWith<std::string> Rule =
- makeRule(callExpr(callee(functionDecl(hasName("f")))),
- change(cat("other()")), cat("no message"));
+ RewriteRule Rule = makeRule(callExpr(callee(functionDecl(hasName("f")))),
+ change(cat("other()")), cat("no message"));
addInclude(Rule, "clang/OtherLib.h", Format);
return Rule;
}
@@ -246,10 +243,10 @@ TEST(TransformerClangTidyCheckTest, AddIncludeAngled) {
}
class IncludeOrderCheck : public TransformerClangTidyCheck {
- static RewriteRuleWith<std::string> rule() {
+ static RewriteRule rule() {
using namespace ::clang::ast_matchers;
- RewriteRuleWith<std::string> Rule = transformer::makeRule(
- integerLiteral(), change(cat("5")), cat("no message"));
+ RewriteRule Rule = transformer::makeRule(integerLiteral(), change(cat("5")),
+ cat("no message"));
addInclude(Rule, "bar.h", IncludeFormat::Quoted);
return Rule;
}
diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index b4609099bf804..6b14861e92d76 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -61,9 +61,7 @@ enum class IncludeFormat {
/// of `EditGenerator`.
using EditGenerator = MatchConsumer<llvm::SmallVector<Edit, 1>>;
-template <typename T> using Generator = std::shared_ptr<MatchComputation<T>>;
-
-using TextGenerator = Generator<std::string>;
+using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
using AnyGenerator = MatchConsumer<llvm::Any>;
@@ -264,9 +262,12 @@ inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) {
//
// * Edits: a set of Edits to the source code, described with ASTEdits.
//
+// * Explanation: explanation of the rewrite. This will be displayed to the
+// user, where possible; for example, in clang-tidy diagnostics.
+//
// However, rules can also consist of (sub)rules, where the first that matches
-// is applied and the rest are ignored. So, the above components together form
-// a logical "case" and a rule is a sequence of cases.
+// is applied and the rest are ignored. So, the above components are gathered
+// as a `Case` and a rule is a list of cases.
//
// Rule cases have an additional, implicit, component: the parameters. These are
// portions of the pattern which are left unspecified, yet bound in the pattern
@@ -274,82 +275,37 @@ inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) {
//
// The \c Transformer class can be used to apply the rewrite rule and obtain the
// corresponding replacements.
-struct RewriteRuleBase {
+struct RewriteRule {
struct Case {
ast_matchers::internal::DynTypedMatcher Matcher;
EditGenerator Edits;
+ TextGenerator Explanation;
};
// We expect RewriteRules will most commonly include only one case.
SmallVector<Case, 1> Cases;
-};
-/// A source-code transformation with accompanying metadata.
-///
-/// When a case of the rule matches, the \c Transformer invokes the
-/// corresponding metadata generator and provides it alongside the edits.
-template <typename MetadataT> struct RewriteRuleWith : RewriteRuleBase {
- SmallVector<Generator<MetadataT>, 1> Metadata;
+ /// DEPRECATED: use `::clang::transformer::RootID` instead.
+ static const llvm::StringRef RootID;
};
-template <> struct RewriteRuleWith<void> : RewriteRuleBase {};
-
-using RewriteRule = RewriteRuleWith<void>;
-
-namespace detail {
-
-RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
- EditGenerator Edits);
-
-template <typename MetadataT>
-RewriteRuleWith<MetadataT> makeRule(ast_matchers::internal::DynTypedMatcher M,
- EditGenerator Edits,
- Generator<MetadataT> Metadata) {
- RewriteRuleWith<MetadataT> R;
- R.Cases = {{std::move(M), std::move(Edits)}};
- R.Metadata = {std::move(Metadata)};
- return R;
-}
-
-inline EditGenerator makeEditGenerator(EditGenerator Edits) { return Edits; }
-EditGenerator makeEditGenerator(llvm::SmallVector<ASTEdit, 1> Edits);
-EditGenerator makeEditGenerator(ASTEdit Edit);
-
-} // namespace detail
-
-/// Constructs a simple \c RewriteRule. \c Edits can be an \c EditGenerator,
-/// multiple \c ASTEdits, or a single \c ASTEdit.
-/// @{
-template <int &..., typename EditsT>
-RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
- EditsT &&Edits) {
- return detail::makeRule(
- std::move(M), detail::makeEditGenerator(std::forward<EditsT>(Edits)));
-}
-
+/// Constructs a simple \c RewriteRule.
RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
- std::initializer_list<ASTEdit> Edits);
-/// @}
-
-/// Overloads of \c makeRule that also generate metadata when matching.
-/// @{
-template <typename MetadataT, int &..., typename EditsT>
-RewriteRuleWith<MetadataT> makeRule(ast_matchers::internal::DynTypedMatcher M,
- EditsT &&Edits,
- Generator<MetadataT> Metadata) {
- return detail::makeRule(
- std::move(M), detail::makeEditGenerator(std::forward<EditsT>(Edits)),
- std::move(Metadata));
+ EditGenerator Edits, TextGenerator Explanation = nullptr);
+
+/// Constructs a \c RewriteRule from multiple `ASTEdit`s.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+ llvm::SmallVector<ASTEdit, 1> Edits,
+ TextGenerator Explanation = nullptr) {
+ return makeRule(std::move(M), editList(std::move(Edits)),
+ std::move(Explanation));
}
-template <typename MetadataT>
-RewriteRuleWith<MetadataT> makeRule(ast_matchers::internal::DynTypedMatcher M,
- std::initializer_list<ASTEdit> Edits,
- Generator<MetadataT> Metadata) {
- return detail::makeRule(std::move(M),
- detail::makeEditGenerator(std::move(Edits)),
- std::move(Metadata));
+/// Overload of \c makeRule for common case of only one edit.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+ ASTEdit Edit,
+ TextGenerator Explanation = nullptr) {
+ return makeRule(std::move(M), edit(std::move(Edit)), std::move(Explanation));
}
-/// @}
/// For every case in Rule, adds an include directive for the given header. The
/// common use is assumed to be a rule with only one case. For example, to
@@ -361,7 +317,7 @@ RewriteRuleWith<MetadataT> makeRule(ast_matchers::internal::DynTypedMatcher M,
/// addInclude(R, "path/to/bar_header.h");
/// addInclude(R, "vector", IncludeFormat::Angled);
/// \endcode
-void addInclude(RewriteRuleBase &Rule, llvm::StringRef Header,
+void addInclude(RewriteRule &Rule, llvm::StringRef Header,
IncludeFormat Format = IncludeFormat::Quoted);
/// Applies the first rule whose pattern matches; other rules are ignored. If
@@ -403,45 +359,7 @@ void addInclude(RewriteRuleBase &Rule, llvm::StringRef Header,
// makeRule(left_call, left_call_action),
// makeRule(right_call, right_call_action)});
// ```
-/// @{
-template <typename MetadataT>
-RewriteRuleWith<MetadataT>
-applyFirst(ArrayRef<RewriteRuleWith<MetadataT>> Rules) {
- RewriteRuleWith<MetadataT> R;
- for (auto &Rule : Rules) {
- assert(Rule.Cases.size() == Rule.Metadata.size() &&
- "mis-match in case and metadata array size");
- R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
- R.Metadata.append(Rule.Metadata.begin(), Rule.Metadata.end());
- }
- return R;
-}
-
-template <>
-RewriteRuleWith<void> applyFirst(ArrayRef<RewriteRuleWith<void>> Rules);
-
-template <typename MetadataT>
-RewriteRuleWith<MetadataT>
-applyFirst(const std::vector<RewriteRuleWith<MetadataT>> &Rules) {
- return applyFirst(llvm::makeArrayRef(Rules));
-}
-
-template <typename MetadataT>
-RewriteRuleWith<MetadataT>
-applyFirst(std::initializer_list<RewriteRuleWith<MetadataT>> Rules) {
- return applyFirst(llvm::makeArrayRef(Rules.begin(), Rules.end()));
-}
-/// @}
-
-/// Converts a \c RewriteRuleWith<T> to a \c RewriteRule by stripping off the
-/// metadata generators.
-template <int &..., typename MetadataT>
-std::enable_if_t<!std::is_same<MetadataT, void>::value, RewriteRule>
-stripMetadata(RewriteRuleWith<MetadataT> Rule) {
- RewriteRule R;
- R.Cases = std::move(Rule.Cases);
- return R;
-}
+RewriteRule applyFirst(ArrayRef<RewriteRule> Rules);
/// Applies `Rule` to all descendants of the node bound to `NodeId`. `Rule` can
/// refer to nodes bound by the calling rule. `Rule` is not applied to the node
@@ -505,8 +423,7 @@ rewriteDescendants(const DynTypedNode &Node, RewriteRule Rule,
/// Only supports Rules whose cases' matchers share the same base "kind"
/// (`Stmt`, `Decl`, etc.) Deprecated: use `buildMatchers` instead, which
/// supports mixing matchers of
diff erent kinds.
-ast_matchers::internal::DynTypedMatcher
-buildMatcher(const RewriteRuleBase &Rule);
+ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
/// Builds a set of matchers that cover the rule.
///
@@ -516,7 +433,7 @@ buildMatcher(const RewriteRuleBase &Rule);
/// for rewriting. If any such matchers are included, will return an empty
/// vector.
std::vector<ast_matchers::internal::DynTypedMatcher>
-buildMatchers(const RewriteRuleBase &Rule);
+buildMatchers(const RewriteRule &Rule);
/// Gets the beginning location of the source matched by a rewrite rule. If the
/// match occurs within a macro expansion, returns the beginning of the
@@ -524,10 +441,11 @@ buildMatchers(const RewriteRuleBase &Rule);
SourceLocation
getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result);
-/// Returns the index of the \c Case of \c Rule that was selected in the match
-/// result. Assumes a matcher built with \c buildMatcher.
-size_t findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
- const RewriteRuleBase &Rule);
+/// Returns the \c Case of \c Rule that was selected in the match result.
+/// Assumes a matcher built with \c buildMatcher.
+const RewriteRule::Case &
+findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
+ const RewriteRule &Rule);
} // namespace detail
} // namespace transformer
} // namespace clang
diff --git a/clang/include/clang/Tooling/Transformer/Transformer.h b/clang/include/clang/Tooling/Transformer/Transformer.h
index 53dd61f35ef5e..3e0a026a7f2aa 100644
--- a/clang/include/clang/Tooling/Transformer/Transformer.h
+++ b/clang/include/clang/Tooling/Transformer/Transformer.h
@@ -18,62 +18,6 @@
namespace clang {
namespace tooling {
-
-namespace detail {
-/// Implementation details of \c Transformer with type erasure around
-/// \c RewriteRule and \c RewriteRule<T> as well as the corresponding consumers.
-class TransformerImpl {
-public:
- virtual ~TransformerImpl() = default;
-
- void onMatch(const ast_matchers::MatchFinder::MatchResult &Result);
-
- virtual std::vector<ast_matchers::internal::DynTypedMatcher>
- buildMatchers() const = 0;
-
-protected:
- /// Converts a set of \c Edit into a \c AtomicChange per file modified.
- /// Returns an error if the edits fail to compose, e.g. overlapping edits.
- static llvm::Expected<llvm::SmallVector<AtomicChange, 1>>
- convertToAtomicChanges(const llvm::SmallVectorImpl<transformer::Edit> &Edits,
- const ast_matchers::MatchFinder::MatchResult &Result);
-
-private:
- virtual void
- onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) = 0;
-};
-
-/// Implementation for when no metadata is generated as a part of the
-/// \c RewriteRule.
-class NoMetadataImpl final : public TransformerImpl {
- transformer::RewriteRule Rule;
- std::function<void(Expected<llvm::MutableArrayRef<AtomicChange>>)> Consumer;
-
-public:
- explicit NoMetadataImpl(
- transformer::RewriteRule R,
- std::function<void(Expected<llvm::MutableArrayRef<AtomicChange>>)>
- Consumer)
- : Rule(std::move(R)), Consumer(std::move(Consumer)) {
- assert(llvm::all_of(Rule.Cases,
- [](const transformer::RewriteRule::Case &Case) {
- return Case.Edits;
- }) &&
- "edit generator must be provided for each rule");
- }
-
-private:
- void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final;
- std::vector<ast_matchers::internal::DynTypedMatcher>
- buildMatchers() const final {
- return transformer::detail::buildMatchers(Rule);
- }
-};
-
-// FIXME: Use std::type_identity or backport when available.
-template <class T> struct type_identity { using type = T; };
-} // namespace detail
-
/// Handles the matcher and callback registration for a single `RewriteRule`, as
/// defined by the arguments of the constructor.
class Transformer : public ast_matchers::MatchFinder::MatchCallback {
@@ -87,38 +31,16 @@ class Transformer : public ast_matchers::MatchFinder::MatchCallback {
using ChangeSetConsumer = std::function<void(
Expected<llvm::MutableArrayRef<AtomicChange>> Changes)>;
- template <typename T> struct Result {
- llvm::MutableArrayRef<AtomicChange> Changes;
- T Metadata;
- };
-
- // Specialization provided only to avoid SFINAE on the Transformer
- // constructor; not intended for use.
- template <> struct Result<void> {
- llvm::MutableArrayRef<AtomicChange> Changes;
- };
-
- /// \param Consumer receives all rewrites for a single match, or an error.
+ /// \param Consumer Receives all rewrites for a single match, or an error.
/// Will not necessarily be called for each match; for example, if the rule
/// generates no edits but does not fail. Note that clients are responsible
/// for handling the case that independent \c AtomicChanges conflict with each
/// other.
- explicit Transformer(transformer::RewriteRuleWith<void> Rule,
+ explicit Transformer(transformer::RewriteRule Rule,
ChangeSetConsumer Consumer)
- : Impl(std::make_unique<detail::NoMetadataImpl>(std::move(Rule),
- std::move(Consumer))) {}
-
- /// \param Consumer receives all rewrites and the associated metadata for a
- /// single match, or an error. Will always be called for each match, even if
- /// the rule generates no edits. Note that clients are responsible for
- /// handling the case that independent \c AtomicChanges conflict with each
- /// other.
- template <typename MetadataT>
- explicit Transformer(
- transformer::RewriteRuleWith<MetadataT> Rule,
- std::function<void(llvm::Expected<Transformer::Result<
- typename detail::type_identity<MetadataT>::type>>)>
- Consumer);
+ : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {
+ assert(this->Consumer && "Consumer is empty");
+ }
/// N.B. Passes `this` pointer to `MatchFinder`. So, this object should not
/// be moved after this call.
@@ -129,77 +51,11 @@ class Transformer : public ast_matchers::MatchFinder::MatchCallback {
void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
- std::unique_ptr<detail::TransformerImpl> Impl;
-};
-
-namespace detail {
-/// Implementation when metadata is generated as a part of the rewrite. This
-/// happens when we have a \c RewriteRuleWith<T>.
-template <typename T> class WithMetadataImpl final : public TransformerImpl {
- transformer::RewriteRuleWith<T> Rule;
- std::function<void(llvm::Expected<Transformer::Result<T>>)> Consumer;
-
-public:
- explicit WithMetadataImpl(
- transformer::RewriteRuleWith<T> R,
- std::function<void(llvm::Expected<Transformer::Result<T>>)> Consumer)
- : Rule(std::move(R)), Consumer(std::move(Consumer)) {
- assert(llvm::all_of(Rule.Cases,
- [](const transformer::RewriteRuleBase::Case &Case)
- -> bool { return !!Case.Edits; }) &&
- "edit generator must be provided for each rule");
- assert(llvm::all_of(Rule.Metadata,
- [](const typename transformer::Generator<T> &Metadata)
- -> bool { return !!Metadata; }) &&
- "metadata generator must be provided for each rule");
- }
-
-private:
- void onMatchImpl(const ast_matchers::MatchFinder::MatchResult &Result) final {
- size_t I = transformer::detail::findSelectedCase(Result, Rule);
- auto Transformations = Rule.Cases[I].Edits(Result);
- if (!Transformations) {
- Consumer(Transformations.takeError());
- return;
- }
-
- llvm::SmallVector<AtomicChange, 1> Changes;
- if (!Transformations->empty()) {
- auto C = convertToAtomicChanges(*Transformations, Result);
- if (C) {
- Changes = std::move(*C);
- } else {
- Consumer(C.takeError());
- return;
- }
- }
-
- auto Metadata = Rule.Metadata[I]->eval(Result);
- if (!Metadata) {
- Consumer(Metadata.takeError());
- return;
- }
-
- Consumer(Transformer::Result<T>{
- llvm::MutableArrayRef<AtomicChange>(Changes), std::move(*Metadata)});
- }
-
- std::vector<ast_matchers::internal::DynTypedMatcher>
- buildMatchers() const final {
- return transformer::detail::buildMatchers(Rule);
- }
+ transformer::RewriteRule Rule;
+ /// Receives sets of successful rewrites as an
+ /// \c llvm::ArrayRef<AtomicChange>.
+ ChangeSetConsumer Consumer;
};
-} // namespace detail
-
-template <typename MetadataT>
-Transformer::Transformer(
- transformer::RewriteRuleWith<MetadataT> Rule,
- std::function<void(llvm::Expected<Transformer::Result<
- typename detail::type_identity<MetadataT>::type>>)>
- Consumer)
- : Impl(std::make_unique<detail::WithMetadataImpl<MetadataT>>(
- std::move(Rule), std::move(Consumer))) {}
-
} // namespace tooling
} // namespace clang
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 3e76489782f34..93bd7e91dba7c 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -166,26 +166,10 @@ ASTEdit transformer::addInclude(RangeSelector Target, StringRef Header,
return E;
}
-EditGenerator
-transformer::detail::makeEditGenerator(llvm::SmallVector<ASTEdit, 1> Edits) {
- return editList(std::move(Edits));
-}
-
-EditGenerator transformer::detail::makeEditGenerator(ASTEdit Edit) {
- return edit(std::move(Edit));
-}
-
-RewriteRule transformer::detail::makeRule(DynTypedMatcher M,
- EditGenerator Edits) {
- RewriteRule R;
- R.Cases = {{std::move(M), std::move(Edits)}};
- return R;
-}
-
-RewriteRule transformer::makeRule(ast_matchers::internal::DynTypedMatcher M,
- std::initializer_list<ASTEdit> Edits) {
- return detail::makeRule(std::move(M),
- detail::makeEditGenerator(std::move(Edits)));
+RewriteRule transformer::makeRule(DynTypedMatcher M, EditGenerator Edits,
+ TextGenerator Explanation) {
+ return RewriteRule{{RewriteRule::Case{std::move(M), std::move(Edits),
+ std::move(Explanation)}}};
}
namespace {
@@ -263,8 +247,9 @@ class ApplyRuleCallback : public MatchFinder::MatchCallback {
void run(const MatchFinder::MatchResult &Result) override {
if (!Edits)
return;
- size_t I = transformer::detail::findSelectedCase(Result, Rule);
- auto Transformations = Rule.Cases[I].Edits(Result);
+ transformer::RewriteRule::Case Case =
+ transformer::detail::findSelectedCase(Result, Rule);
+ auto Transformations = Case.Edits(Result);
if (!Transformations) {
Edits = Transformations.takeError();
return;
@@ -340,7 +325,7 @@ EditGenerator transformer::rewriteDescendants(std::string NodeId,
};
}
-void transformer::addInclude(RewriteRuleBase &Rule, StringRef Header,
+void transformer::addInclude(RewriteRule &Rule, StringRef Header,
IncludeFormat Format) {
for (auto &Case : Rule.Cases)
Case.Edits = flatten(std::move(Case.Edits), addInclude(Header, Format));
@@ -381,9 +366,7 @@ static std::vector<DynTypedMatcher> taggedMatchers(
// Simply gathers the contents of the various rules into a single rule. The
// actual work to combine these into an ordered choice is deferred to matcher
// registration.
-template <>
-RewriteRuleWith<void>
-transformer::applyFirst(ArrayRef<RewriteRuleWith<void>> Rules) {
+RewriteRule transformer::applyFirst(ArrayRef<RewriteRule> Rules) {
RewriteRule R;
for (auto &Rule : Rules)
R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
@@ -391,13 +374,12 @@ transformer::applyFirst(ArrayRef<RewriteRuleWith<void>> Rules) {
}
std::vector<DynTypedMatcher>
-transformer::detail::buildMatchers(const RewriteRuleBase &Rule) {
+transformer::detail::buildMatchers(const RewriteRule &Rule) {
// Map the cases into buckets of matchers -- one for each "root" AST kind,
// which guarantees that they can be combined in a single anyOf matcher. Each
// case is paired with an identifying number that is converted to a string id
// in `taggedMatchers`.
- std::map<ASTNodeKind,
- SmallVector<std::pair<size_t, RewriteRuleBase::Case>, 1>>
+ std::map<ASTNodeKind, SmallVector<std::pair<size_t, RewriteRule::Case>, 1>>
Buckets;
const SmallVectorImpl<RewriteRule::Case> &Cases = Rule.Cases;
for (int I = 0, N = Cases.size(); I < N; ++I) {
@@ -423,7 +405,7 @@ transformer::detail::buildMatchers(const RewriteRuleBase &Rule) {
return Matchers;
}
-DynTypedMatcher transformer::detail::buildMatcher(const RewriteRuleBase &Rule) {
+DynTypedMatcher transformer::detail::buildMatcher(const RewriteRule &Rule) {
std::vector<DynTypedMatcher> Ms = buildMatchers(Rule);
assert(Ms.size() == 1 && "Cases must have compatible matchers.");
return Ms[0];
@@ -446,16 +428,19 @@ SourceLocation transformer::detail::getRuleMatchLoc(const MatchResult &Result) {
// Finds the case that was "selected" -- that is, whose matcher triggered the
// `MatchResult`.
-size_t transformer::detail::findSelectedCase(const MatchResult &Result,
- const RewriteRuleBase &Rule) {
+const RewriteRule::Case &
+transformer::detail::findSelectedCase(const MatchResult &Result,
+ const RewriteRule &Rule) {
if (Rule.Cases.size() == 1)
- return 0;
+ return Rule.Cases[0];
auto &NodesMap = Result.Nodes.getMap();
for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) {
std::string Tag = ("Tag" + Twine(i)).str();
if (NodesMap.find(Tag) != NodesMap.end())
- return i;
+ return Rule.Cases[i];
}
llvm_unreachable("No tag found for this rule.");
}
+
+const llvm::StringRef RewriteRule::RootID = ::clang::transformer::RootID;
diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp
index 1cf824d2cbe49..1219afc551868 100644
--- a/clang/lib/Tooling/Transformer/Transformer.cpp
+++ b/clang/lib/Tooling/Transformer/Transformer.cpp
@@ -16,29 +16,35 @@
#include <utility>
#include <vector>
-namespace clang {
-namespace tooling {
+using namespace clang;
+using namespace tooling;
-using ::clang::ast_matchers::MatchFinder;
+using ast_matchers::MatchFinder;
-namespace detail {
+void Transformer::registerMatchers(MatchFinder *MatchFinder) {
+ for (auto &Matcher : transformer::detail::buildMatchers(Rule))
+ MatchFinder->addDynamicMatcher(Matcher, this);
+}
-void TransformerImpl::onMatch(
- const ast_matchers::MatchFinder::MatchResult &Result) {
+void Transformer::run(const MatchFinder::MatchResult &Result) {
if (Result.Context->getDiagnostics().hasErrorOccurred())
return;
- onMatchImpl(Result);
-}
+ transformer::RewriteRule::Case Case =
+ transformer::detail::findSelectedCase(Result, Rule);
+ auto Transformations = Case.Edits(Result);
+ if (!Transformations) {
+ Consumer(Transformations.takeError());
+ return;
+ }
+
+ if (Transformations->empty())
+ return;
-llvm::Expected<llvm::SmallVector<AtomicChange, 1>>
-TransformerImpl::convertToAtomicChanges(
- const llvm::SmallVectorImpl<transformer::Edit> &Edits,
- const MatchFinder::MatchResult &Result) {
// Group the transformations, by file, into AtomicChanges, each anchored by
// the location of the first change in that file.
std::map<FileID, AtomicChange> ChangesByFileID;
- for (const auto &T : Edits) {
+ for (const auto &T : *Transformations) {
auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
auto Iter = ChangesByFileID
.emplace(ID, AtomicChange(*Result.SourceManager,
@@ -49,7 +55,8 @@ TransformerImpl::convertToAtomicChanges(
case transformer::EditKind::Range:
if (auto Err =
AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
- return std::move(Err);
+ Consumer(std::move(Err));
+ return;
}
break;
case transformer::EditKind::AddInclude:
@@ -62,43 +69,5 @@ TransformerImpl::convertToAtomicChanges(
Changes.reserve(ChangesByFileID.size());
for (auto &IDChangePair : ChangesByFileID)
Changes.push_back(std::move(IDChangePair.second));
-
- return Changes;
-}
-
-void NoMetadataImpl::onMatchImpl(const MatchFinder::MatchResult &Result) {
- size_t I = transformer::detail::findSelectedCase(Result, Rule);
- auto Transformations = Rule.Cases[I].Edits(Result);
- if (!Transformations) {
- Consumer(Transformations.takeError());
- return;
- }
-
- if (Transformations->empty())
- return;
-
- auto Changes = convertToAtomicChanges(*Transformations, Result);
- if (!Changes) {
- Consumer(Changes.takeError());
- return;
- }
-
- Consumer(llvm::MutableArrayRef<AtomicChange>(*Changes));
+ Consumer(llvm::MutableArrayRef<AtomicChange>(Changes));
}
-
-} // namespace detail
-
-void Transformer::registerMatchers(MatchFinder *MatchFinder) {
- for (auto &Matcher : Impl->buildMatchers())
- MatchFinder->addDynamicMatcher(Matcher, this);
-}
-
-void Transformer::run(const MatchFinder::MatchResult &Result) {
- if (Result.Context->getDiagnostics().hasErrorOccurred())
- return;
-
- Impl->onMatch(Result);
-}
-
-} // namespace tooling
-} // namespace clang
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index 9ca1daa0e1e74..4ab39843de935 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -31,11 +31,9 @@ using ::clang::transformer::makeRule;
using ::clang::transformer::member;
using ::clang::transformer::name;
using ::clang::transformer::node;
-using ::clang::transformer::noEdits;
using ::clang::transformer::remove;
using ::clang::transformer::rewriteDescendants;
using ::clang::transformer::RewriteRule;
-using ::clang::transformer::RewriteRuleWith;
using ::clang::transformer::statement;
using ::testing::ElementsAre;
using ::testing::IsEmpty;
@@ -131,7 +129,7 @@ class ClangRefactoringTestBase : public testing::Test {
Changes.insert(Changes.end(), std::make_move_iterator(C->begin()),
std::make_move_iterator(C->end()));
} else {
- // FIXME: stash this error rather than printing.
+ // FIXME: stash this error rather then printing.
llvm::errs() << "Error generating changes: "
<< llvm::toString(C.takeError()) << "\n";
++ErrorCount;
@@ -139,58 +137,27 @@ class ClangRefactoringTestBase : public testing::Test {
};
}
- auto consumerWithStringMetadata() {
- return [this](Expected<Transformer::Result<std::string>> C) {
- if (C) {
- Changes.insert(Changes.end(),
- std::make_move_iterator(C->Changes.begin()),
- std::make_move_iterator(C->Changes.end()));
- StringMetadata.push_back(std::move(C->Metadata));
- } else {
- // FIXME: stash this error rather than printing.
- llvm::errs() << "Error generating changes: "
- << llvm::toString(C.takeError()) << "\n";
- ++ErrorCount;
- }
- };
- }
-
- void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+ template <typename R>
+ void testRule(R Rule, StringRef Input, StringRef Expected) {
Transformers.push_back(
std::make_unique<Transformer>(std::move(Rule), consumer()));
Transformers.back()->registerMatchers(&MatchFinder);
compareSnippets(Expected, rewrite(Input));
}
- void testRule(RewriteRuleWith<std::string> Rule, StringRef Input,
- StringRef Expected) {
- Transformers.push_back(std::make_unique<Transformer>(
- std::move(Rule), consumerWithStringMetadata()));
- Transformers.back()->registerMatchers(&MatchFinder);
- compareSnippets(Expected, rewrite(Input));
- }
-
- void testRuleFailure(RewriteRule Rule, StringRef Input) {
+ template <typename R> void testRuleFailure(R Rule, StringRef Input) {
Transformers.push_back(
std::make_unique<Transformer>(std::move(Rule), consumer()));
Transformers.back()->registerMatchers(&MatchFinder);
ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
}
- void testRuleFailure(RewriteRuleWith<std::string> Rule, StringRef Input) {
- Transformers.push_back(std::make_unique<Transformer>(
- std::move(Rule), consumerWithStringMetadata()));
- Transformers.back()->registerMatchers(&MatchFinder);
- ASSERT_FALSE(rewrite(Input)) << "Expected failure to rewrite code";
- }
-
// Transformers are referenced by MatchFinder.
std::vector<std::unique_ptr<Transformer>> Transformers;
clang::ast_matchers::MatchFinder MatchFinder;
// Records whether any errors occurred in individual changes.
int ErrorCount = 0;
AtomicChanges Changes;
- std::vector<std::string> StringMetadata;
private:
FileContentMappings FileContents = {{"header.h", ""}};
@@ -202,7 +169,7 @@ class TransformerTest : public ClangRefactoringTestBase {
};
// Given string s, change strlen($s.c_str()) to REPLACED.
-static RewriteRuleWith<std::string> ruleStrlenSize() {
+static RewriteRule ruleStrlenSize() {
StringRef StringExpr = "strexpr";
auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
auto R = makeRule(
@@ -919,12 +886,12 @@ TEST_F(TransformerTest, FlattenWithMixedArgs) {
TEST_F(TransformerTest, OrderedRuleUnrelated) {
StringRef Flag = "flag";
- RewriteRuleWith<std::string> FlagRule = makeRule(
+ RewriteRule FlagRule = makeRule(
cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
hasName("proto::ProtoCommandLineFlag"))))
.bind(Flag)),
unless(callee(cxxMethodDecl(hasName("GetProto"))))),
- changeTo(node(std::string(Flag)), cat("PROTO")), cat(""));
+ changeTo(node(std::string(Flag)), cat("PROTO")));
std::string Input = R"cc(
proto::ProtoCommandLineFlag flag;
@@ -1690,8 +1657,8 @@ TEST_F(TransformerTest, MultiFileEdit) {
makeRule(callExpr(callee(functionDecl(hasName("Func"))),
forEachArgumentWithParam(expr().bind("arg"),
parmVarDecl().bind("param"))),
- {changeTo(node("arg"), cat("ARG")),
- changeTo(node("param"), cat("PARAM"))}),
+ editList({changeTo(node("arg"), cat("ARG")),
+ changeTo(node("param"), cat("PARAM"))})),
[&](Expected<MutableArrayRef<AtomicChange>> Changes) {
if (Changes)
ChangeSets.push_back(AtomicChanges(Changes->begin(), Changes->end()));
@@ -1715,39 +1682,4 @@ TEST_F(TransformerTest, MultiFileEdit) {
"./input.h"))));
}
-TEST_F(TransformerTest, GeneratesMetadata) {
- std::string Input = R"cc(int target = 0;)cc";
- std::string Expected = R"cc(REPLACE)cc";
- RewriteRuleWith<std::string> Rule = makeRule(
- varDecl(hasName("target")), changeTo(cat("REPLACE")), cat("METADATA"));
- testRule(std::move(Rule), Input, Expected);
- EXPECT_EQ(ErrorCount, 0);
- EXPECT_THAT(StringMetadata, UnorderedElementsAre("METADATA"));
-}
-
-TEST_F(TransformerTest, GeneratesMetadataWithNoEdits) {
- std::string Input = R"cc(int target = 0;)cc";
- RewriteRuleWith<std::string> Rule = makeRule(
- varDecl(hasName("target")).bind("var"), noEdits(), cat("METADATA"));
- testRule(std::move(Rule), Input, Input);
- EXPECT_EQ(ErrorCount, 0);
- EXPECT_THAT(StringMetadata, UnorderedElementsAre("METADATA"));
-}
-
-TEST_F(TransformerTest, PropagateMetadataErrors) {
- class AlwaysFail : public transformer::MatchComputation<std::string> {
- llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,
- std::string *) const override {
- return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
- }
- std::string toString() const override { return "AlwaysFail"; }
- };
- std::string Input = R"cc(int target = 0;)cc";
- RewriteRuleWith<std::string> Rule = makeRule<std::string>(
- varDecl(hasName("target")).bind("var"), changeTo(cat("REPLACE")),
- std::make_shared<AlwaysFail>());
- testRuleFailure(std::move(Rule), Input);
- EXPECT_EQ(ErrorCount, 1);
-}
-
} // namespace
More information about the cfe-commits
mailing list