[clang-tools-extra] 489449c - [libTooling] Further simplify `Stencil` type and introduce `MatchComputation`.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 11 09:45:36 PST 2019
Author: Yitzhak Mandelbaum
Date: 2019-11-11T12:44:15-05:00
New Revision: 489449c28aaa45086d507fbad96826420adf409d
URL: https://github.com/llvm/llvm-project/commit/489449c28aaa45086d507fbad96826420adf409d
DIFF: https://github.com/llvm/llvm-project/commit/489449c28aaa45086d507fbad96826420adf409d.diff
LOG: [libTooling] Further simplify `Stencil` type and introduce `MatchComputation`.
Summary:
This revision introduces a new interface `MatchComputation` which generalizes
the `Stencil` interface and replaces the `std::function` interface of
`MatchConsumer`. With this revision, `Stencil` (as an abstraction) becomes just
one collection of implementations of
`MatchComputation<std::string>`. Correspondingly, we remove the `Stencil` class
entirely in favor of a simple type alias, deprecate `MatchConsumer` and change
all functions that accepted `MatchConsumer<std::string>` to use
`MatchComputation<std::string>` instead.
Reviewers: gribozavr
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D69802
Added:
Modified:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang/include/clang/Tooling/Transformer/MatchConsumer.h
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/include/clang/Tooling/Transformer/Stencil.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/lib/Tooling/Transformer/Stencil.cpp
clang/unittests/Tooling/StencilTest.cpp
clang/unittests/Tooling/TransformerTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index 1c31cf56c68a..515997b14231 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -85,7 +85,7 @@ void TransformerClangTidyCheck::check(
if (Transformations->empty())
return;
- Expected<std::string> Explanation = Case.Explanation(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/include/clang/Tooling/Transformer/MatchConsumer.h b/clang/include/clang/Tooling/Transformer/MatchConsumer.h
index 0a1dbe13ea1e..f407ffce3d25 100644
--- a/clang/include/clang/Tooling/Transformer/MatchConsumer.h
+++ b/clang/include/clang/Tooling/Transformer/MatchConsumer.h
@@ -51,6 +51,53 @@ MatchConsumer<T> ifBound(std::string ID, MatchConsumer<T> TrueC,
return (Map.find(ID) != Map.end() ? TrueC : FalseC)(Result);
};
}
+
+/// A failable computation over nodes bound by AST matchers, with (limited)
+/// reflection via the `toString` method.
+///
+/// The computation should report any errors though its return value (rather
+/// than terminating the program) to enable usage in interactive scenarios like
+/// clang-query.
+///
+/// This is a central abstraction of the Transformer framework. It is a
+/// generalization of `MatchConsumer` and intended to replace it.
+template <typename T> class MatchComputation {
+public:
+ virtual ~MatchComputation() = default;
+
+ /// Evaluates the computation and (potentially) updates the accumulator \c
+ /// Result. \c Result is undefined in the case of an error. `Result` is an
+ /// out parameter to optimize case where the computation involves composing
+ /// the result of sub-computation evaluations.
+ virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+ T *Result) const = 0;
+
+ /// Convenience version of `eval`, for the case where the computation is being
+ /// evaluated on its own.
+ llvm::Expected<T> eval(const ast_matchers::MatchFinder::MatchResult &R) const;
+
+ /// Constructs a string representation of the computation, for informational
+ /// purposes. The representation must be deterministic, but is not required to
+ /// be unique.
+ virtual std::string toString() const = 0;
+
+protected:
+ MatchComputation() = default;
+
+ // Since this is an abstract class, copying/assigning only make sense for
+ // derived classes implementing `clone()`.
+ MatchComputation(const MatchComputation &) = default;
+ MatchComputation &operator=(const MatchComputation &) = default;
+};
+
+template <typename T>
+llvm::Expected<T> MatchComputation<T>::eval(
+ const ast_matchers::MatchFinder::MatchResult &R) const {
+ T Output;
+ if (auto Err = eval(R, &Output))
+ return std::move(Err);
+ return Output;
+}
} // namespace transformer
namespace tooling {
diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index 11ff2bc9867f..7daf6ea154be 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -30,7 +30,7 @@
namespace clang {
namespace transformer {
-using TextGenerator = MatchConsumer<std::string>;
+using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
// Description of a source-code edit, expressed in terms of an AST node.
// Includes: an ID for the (bound) node, a selector for source related to the
@@ -223,11 +223,7 @@ inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
}
/// Removes the source selected by \p S.
-inline ASTEdit remove(RangeSelector S) {
- return change(std::move(S),
- [](const ast_matchers::MatchFinder::MatchResult &)
- -> Expected<std::string> { return ""; });
-}
+ASTEdit remove(RangeSelector S);
/// The following three functions are a low-level part of the RewriteRule
/// API. We expose them for use in implementing the fixtures that interpret
@@ -294,10 +290,7 @@ namespace tooling {
/// Wraps a string as a TextGenerator.
using TextGenerator = transformer::TextGenerator;
-inline TextGenerator text(std::string M) {
- return [M](const ast_matchers::MatchFinder::MatchResult &)
- -> Expected<std::string> { return M; };
-}
+TextGenerator text(std::string M);
using transformer::addInclude;
using transformer::applyFirst;
diff --git a/clang/include/clang/Tooling/Transformer/Stencil.h b/clang/include/clang/Tooling/Transformer/Stencil.h
index 5c1139875a19..dd65e68b7e8a 100644
--- a/clang/include/clang/Tooling/Transformer/Stencil.h
+++ b/clang/include/clang/Tooling/Transformer/Stencil.h
@@ -32,33 +32,8 @@
namespace clang {
namespace transformer {
-class StencilInterface {
-public:
- virtual ~StencilInterface() = default;
-
- /// Evaluates this part to a string and appends it to \c Result. \c Result is
- /// undefined in the case of an error. `Result` is an out parameter to
- /// optimize the expected common case of evaluating a sequence of generators.
- virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
- std::string *Result) const = 0;
-
- /// Convenience version of `eval`, for the case where the generator is being
- /// evaluated on its own.
- llvm::Expected<std::string>
- eval(const ast_matchers::MatchFinder::MatchResult &R) const;
-
- /// Constructs a string representation of the StencilInterface. The
- /// representation must be deterministic, but is not required to be unique.
- virtual std::string toString() const = 0;
-
-protected:
- StencilInterface() = default;
-
- // Since this is an abstract class, copying/assigning only make sense for
- // derived classes implementing `clone()`.
- StencilInterface(const StencilInterface &) = default;
- StencilInterface &operator=(const StencilInterface &) = default;
-};
+
+using StencilInterface = MatchComputation<std::string>;
/// A sequence of code fragments, references to parameters and code-generation
/// operations that together can be evaluated to (a fragment of) source code or
@@ -68,27 +43,13 @@ class StencilInterface {
/// Since `StencilInterface` is an immutable interface, the sharing doesn't
/// impose any risks. Otherwise, we would have to add a virtual `copy` method to
/// the API and implement it for all derived classes.
-class Stencil {
- std::shared_ptr<StencilInterface> Impl;
-
-public:
- Stencil() = default;
- explicit Stencil(std::shared_ptr<StencilInterface> Ptr)
- : Impl(std::move(Ptr)) {}
-
- StencilInterface *operator->() const { return Impl.get(); }
-
- llvm::Expected<std::string>
- operator()(const ast_matchers::MatchFinder::MatchResult &R) {
- return Impl->eval(R);
- }
-};
+using Stencil = std::shared_ptr<StencilInterface>;
namespace detail {
/// Convenience function to construct a \c Stencil. Overloaded for common cases
/// so that user doesn't need to specify which factory function to use. This
/// pattern gives benefits similar to implicit constructors, while maintaing a
-/// higher degree of explictness.
+/// higher degree of explicitness.
Stencil makeStencil(llvm::StringRef Text);
Stencil makeStencil(RangeSelector Selector);
inline Stencil makeStencil(Stencil S) { return S; }
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index b96b5eeabad0..20d3a371950a 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -43,7 +43,7 @@ transformer::detail::translateEdits(const MatchResult &Result,
// it as is currently done.
if (!EditRange)
return SmallVector<Transformation, 0>();
- auto Replacement = Edit.Replacement(Result);
+ auto Replacement = Edit.Replacement->eval(Result);
if (!Replacement)
return Replacement.takeError();
transformer::detail::Transformation T;
@@ -61,6 +61,28 @@ ASTEdit transformer::changeTo(RangeSelector S, TextGenerator Replacement) {
return E;
}
+namespace {
+/// A \c TextGenerator that always returns a fixed string.
+class SimpleTextGenerator : public MatchComputation<std::string> {
+ std::string S;
+
+public:
+ SimpleTextGenerator(std::string S) : S(std::move(S)) {}
+ llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,
+ std::string *Result) const override {
+ Result->append(S);
+ return llvm::Error::success();
+ }
+ std::string toString() const override {
+ return (llvm::Twine("text(\"") + S + "\")").str();
+ }
+};
+} // namespace
+
+ASTEdit transformer::remove(RangeSelector S) {
+ return change(std::move(S), std::make_shared<SimpleTextGenerator>(""));
+}
+
RewriteRule transformer::makeRule(DynTypedMatcher M, SmallVector<ASTEdit, 1> Edits,
TextGenerator Explanation) {
return RewriteRule{{RewriteRule::Case{
@@ -176,3 +198,7 @@ transformer::detail::findSelectedCase(const MatchResult &Result,
}
constexpr llvm::StringLiteral RewriteRule::RootID;
+
+TextGenerator tooling::text(std::string M) {
+ return std::make_shared<SimpleTextGenerator>(std::move(M));
+}
diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp b/clang/lib/Tooling/Transformer/Stencil.cpp
index d994fdcf8707..486e18b341f7 100644
--- a/clang/lib/Tooling/Transformer/Stencil.cpp
+++ b/clang/lib/Tooling/Transformer/Stencil.cpp
@@ -272,14 +272,6 @@ template <typename T> class StencilImpl : public StencilInterface {
};
} // namespace
-llvm::Expected<std::string>
-StencilInterface::eval(const MatchFinder::MatchResult &R) const {
- std::string Output;
- if (auto Err = eval(R, &Output))
- return std::move(Err);
- return Output;
-}
-
Stencil transformer::detail::makeStencil(StringRef Text) { return text(Text); }
Stencil transformer::detail::makeStencil(RangeSelector Selector) {
@@ -287,52 +279,50 @@ Stencil transformer::detail::makeStencil(RangeSelector Selector) {
}
Stencil transformer::text(StringRef Text) {
- return Stencil(std::make_shared<StencilImpl<RawTextData>>(Text));
+ return std::make_shared<StencilImpl<RawTextData>>(Text);
}
Stencil transformer::selection(RangeSelector Selector) {
- return Stencil(
- std::make_shared<StencilImpl<SelectorData>>(std::move(Selector)));
+ return std::make_shared<StencilImpl<SelectorData>>(std::move(Selector));
}
Stencil transformer::dPrint(StringRef Id) {
- return Stencil(std::make_shared<StencilImpl<DebugPrintNodeData>>(Id));
+ return std::make_shared<StencilImpl<DebugPrintNodeData>>(Id);
}
Stencil transformer::expression(llvm::StringRef Id) {
- return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
- UnaryNodeOperator::Parens, Id));
+ return std::make_shared<StencilImpl<UnaryOperationData>>(
+ UnaryNodeOperator::Parens, Id);
}
Stencil transformer::deref(llvm::StringRef ExprId) {
- return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
- UnaryNodeOperator::Deref, ExprId));
+ return std::make_shared<StencilImpl<UnaryOperationData>>(
+ UnaryNodeOperator::Deref, ExprId);
}
Stencil transformer::addressOf(llvm::StringRef ExprId) {
- return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
- UnaryNodeOperator::Address, ExprId));
+ return std::make_shared<StencilImpl<UnaryOperationData>>(
+ UnaryNodeOperator::Address, ExprId);
}
Stencil transformer::access(StringRef BaseId, Stencil Member) {
- return Stencil(
- std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member)));
+ return std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member));
}
Stencil transformer::ifBound(StringRef Id, Stencil TrueStencil,
Stencil FalseStencil) {
- return Stencil(std::make_shared<StencilImpl<IfBoundData>>(
- Id, std::move(TrueStencil), std::move(FalseStencil)));
+ return std::make_shared<StencilImpl<IfBoundData>>(Id, std::move(TrueStencil),
+ std::move(FalseStencil));
}
Stencil transformer::run(MatchConsumer<std::string> Fn) {
- return Stencil(
- std::make_shared<StencilImpl<MatchConsumer<std::string>>>(std::move(Fn)));
+ return std::make_shared<StencilImpl<MatchConsumer<std::string>>>(
+ std::move(Fn));
}
Stencil transformer::catVector(std::vector<Stencil> Parts) {
// Only one argument, so don't wrap in sequence.
if (Parts.size() == 1)
return std::move(Parts[0]);
- return Stencil(std::make_shared<StencilImpl<SequenceData>>(std::move(Parts)));
+ return std::make_shared<StencilImpl<SequenceData>>(std::move(Parts));
}
diff --git a/clang/unittests/Tooling/StencilTest.cpp b/clang/unittests/Tooling/StencilTest.cpp
index 5bbdfea398ab..7f530fe9fbf6 100644
--- a/clang/unittests/Tooling/StencilTest.cpp
+++ b/clang/unittests/Tooling/StencilTest.cpp
@@ -24,7 +24,6 @@ using ::llvm::Failed;
using ::llvm::HasValue;
using ::llvm::StringError;
using ::testing::AllOf;
-using ::testing::Eq;
using ::testing::HasSubstr;
using MatchResult = MatchFinder::MatchResult;
@@ -135,26 +134,6 @@ TEST_F(StencilTest, SingleStatement) {
HasValue("if (!true) return 0; else return 1;"));
}
-// Tests `stencil`.
-TEST_F(StencilTest, StencilFactoryFunction) {
- StringRef Condition("C"), Then("T"), Else("E");
- const std::string Snippet = R"cc(
- if (true)
- return 1;
- else
- return 0;
- )cc";
- auto StmtMatch = matchStmt(
- Snippet, ifStmt(hasCondition(expr().bind(Condition)),
- hasThen(stmt().bind(Then)), hasElse(stmt().bind(Else))));
- ASSERT_TRUE(StmtMatch);
- // Invert the if-then-else.
- auto Consumer = cat("if (!", node(Condition), ") ", statement(Else), " else ",
- statement(Then));
- EXPECT_THAT_EXPECTED(Consumer(StmtMatch->Result),
- HasValue("if (!true) return 0; else return 1;"));
-}
-
TEST_F(StencilTest, UnboundNode) {
const std::string Snippet = R"cc(
if (true)
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index c99bf974f88e..c382783319b8 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -575,13 +575,16 @@ TEST_F(TransformerTest, TextGeneratorFailure) {
std::string Input = "int conflictOneRule() { return 3 + 7; }";
// Try to change the whole binary-operator expression AND one its operands:
StringRef O = "O";
- auto AlwaysFail = [](const ast_matchers::MatchFinder::MatchResult &)
- -> llvm::Expected<std::string> {
- return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+ 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"; }
};
- Transformer T(
- makeRule(binaryOperator().bind(O), changeTo(node(O), AlwaysFail)),
- consumer());
+ Transformer T(makeRule(binaryOperator().bind(O),
+ changeTo(node(O), std::make_shared<AlwaysFail>())),
+ consumer());
T.registerMatchers(&MatchFinder);
EXPECT_FALSE(rewrite(Input));
EXPECT_THAT(Changes, IsEmpty());
More information about the cfe-commits
mailing list