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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
