[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:45
+  /// Evaluates this part to a string and appends it to `result`.
+  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+                           std::string *Result) const = 0;
----------------
Why not use `llvm::Expected<std::string>` as the return type?


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:75
+  // Copy constructor/assignment produce a deep copy.
+  StencilPart(const StencilPart &P) : Impl(P.Impl->clone()) {}
+  StencilPart(StencilPart &&) = default;
----------------
The interface API is all const. Why not keep a `shared_ptr` instead?
That way we don't have to have users implement a clone function.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:94
+      return false;
+    return Impl->isEqual(*(Other.Impl));
+  }
----------------
Parens around Other.Impl are not necessary.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:110
+  template <typename... Ts> static Stencil cat(Ts &&... Parts) {
+    Stencil Stencil;
+    Stencil.Parts.reserve(sizeof...(Parts));
----------------
I don't like naming the variable the same as the type.
You could just use `S` as the variable name. That is ok with llvm style for small functions.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:127
+  // Allow Stencils to operate as std::function, for compatibility with
+  // Transformer's TextGenerator. Calls `eval()` and asserts on failure.
+  std::string operator()(const ast_matchers::MatchFinder::MatchResult &) const;
----------------
"asserts" as it only fails in debug mode?


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:130
+
+  bool operator==(const Stencil &Other) const {
+    return Parts == Other.Parts;
----------------
I usually prefer free functions for binary operators (friends is fine).
It makes them symmetric.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:155
+/// statement.
+StencilPart snode(llvm::StringRef Id);
+
----------------
`sNode` ?
ie camelCase


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:45
+
+namespace {
+// An arbitrary fragment of code within a stencil.
----------------
maybe this anon namespace should cover the functions above?


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:76
+
+static bool operator==(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;
----------------
If you define ==, imo you should define != too.
Otherwise just call it `isEqual`


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:94
+Error evalData(const T &Data, const MatchFinder::MatchResult &Match,
+           std::string *Result);
+
----------------
alignment.
(below too)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59371/new/

https://reviews.llvm.org/D59371