gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:57
+ /// Constructs a string representation of the StencilInterfacePart.
+ /// StencilInterfaceParts generated by the `selection` and `run` functions do
+ /// not have a unique string representation.
----------------
s/Part// (2x)
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:82
+Stencil makeStencil(RangeSelector Selector);
+inline Stencil makeStencil(Stencil S) { return S; }
----------------
I feel like this should be an implementation detail of `cat` (or other stencil
combinators who want to support such implicit conversions). Users writing
stencils should use concrete factory functions below.
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:137
+/// number.
+Stencil sequence(std::vector<Stencil> Parts);
----------------
It is the same operation as `cat`, right? So WDYT about `cat_vector`?
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:144
-namespace tooling {
-// DEPRECATED: These are temporary aliases supporting client migration to the
-// `transformer` namespace.
-using Stencil = transformer::Stencil;
-using StencilPart = transformer::StencilPart;
-namespace stencil {
-using transformer::access;
-using transformer::addressOf;
-using transformer::cat;
-using transformer::deref;
-using transformer::dPrint;
-using transformer::expression;
-using transformer::ifBound;
-using transformer::run;
-using transformer::selection;
-using transformer::text;
-/// \returns the source corresponding to the identified node.
-/// FIXME: Deprecated. Write `selection(node(Id))` instead.
-inline transformer::StencilPart node(llvm::StringRef Id) {
- return selection(tooling::node(Id));
+/// General-purpose Stencil factory function. Concatenates 0+ stencil pieces
+/// into a single stencil. Arguments can be raw text, ranges in the matched
code
----------------
I think it is better to remove the first sentence.
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:150
}
-} // namespace stencil
-} // namespace tooling
+Stencil cat(std::vector<Stencil> Parts);
+
----------------
Like I said in the other review, I don't feel great about an overload set with
universal references and anything else.
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:152
+
+/// Convenience function for creating a stencil-based MatchConsumer. See `cat`
+/// for more details.
----------------
"Creates a MatchConsumer that evaluates a given Stencil."
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:154
+/// for more details.
+template <typename... Ts> MatchConsumer<std::string> stencil(Ts &&... Parts) {
+ Stencil S = cat(std::forward<Ts>(Parts)...);
----------------
I'm a bit confused by the name. The function returns a MatchConsumer, but it is
called "stencil". What is the intended usage?
================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:155
+template <typename... Ts> MatchConsumer<std::string> stencil(Ts &&... Parts) {
+ Stencil S = cat(std::forward<Ts>(Parts)...);
+ return [S](const ast_matchers::MatchFinder::MatchResult &R) {
----------------
I'm not sure the convenience of being able to avoid to say "cat(...)" at the
callsite is worth the API complexity. In other words, my suggestion is to make
this function take a single stencil.
================
Comment at: clang/lib/Tooling/Transformer/Stencil.cpp:97
+// StencilInteface or just Stencil?
+// unique_ptr or shared_ptr?
+
----------------
I think these comments can be deleted now.
================
Comment at: clang/lib/Tooling/Transformer/Stencil.cpp:262
+
+template <typename T> class StencilImpl : public StencilInterface {
T Data;
----------------
There isn't much logic left in StencilImpl.
Would the code be simpler if we made the various Data classes implement
StencilInterface directly?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69613/new/
https://reviews.llvm.org/D69613
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
