[PATCH] D69613: [libTooling] Simplify type structure of `Stencil`s.

[Dmitri Gribenko via Phabricator via cfe-commits]

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