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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for the review. I've changed most of the things with only a few open questions. PTAL.



================
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;
----------------
sbenza wrote:
> Why not use `llvm::Expected<std::string>` as the return type?
so that each stencil part can append to a single string that we construct when evaluating the whole stencil.  this was an (attempted?) optimization. if concatenating is about as efficient, I'd prefer that approach. What do you advise?


================
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;
----------------
sbenza wrote:
> 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.
Excellent! I guess I'm allergic to shared_ptr because of its atomics but its clearly a win here. Thanks!


================
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;
----------------
sbenza wrote:
> "asserts" as it only fails in debug mode?
I thought `assert()` also fails in normal mode, based on my reading of the llvm docs, but it's not explicit about this: http://llvm.org/docs/ProgrammersManual.html#programmatic-errors

FWIW, I'm on the fence whether `eval()` should actually have the same signature and similarly just crash the program on errors. Its not clear there's any value to propogating the errors up the stack via `Expected`.


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:45
+
+namespace {
+// An arbitrary fragment of code within a stencil.
----------------
sbenza wrote:
> maybe this anon namespace should cover the functions above?
the style guide advises against. I've been told to only put type definitions inside anon namespaces.


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:76
+
+static bool operator==(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;
----------------
sbenza wrote:
> If you define ==, imo you should define != too.
> Otherwise just call it `isEqual`
Since I don't have a general need for these as operators, just went w/ the latter. Used the same style as `evalData()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371