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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
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:
> ymandel wrote:
> > 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`.
> `assert` follows `NDEBUG`, which is not explicitly related to `-Ox`, but they usually go together.
> 
> Don't make it crash, please.
> Otherwise it would be hard to have a refactoring service or alike.
Right, you've convinced me. For some background: TextGenerator (in Transformer) originally return an Expected<std::string>, but Ilya suggested that I simplify to just a string and use assertions.  The logic being that these are compiler-style tools. However, I agree that this simply doesn't work for running these in servers, which is a very real concern. It's also trivial to imagine a user mistyping a stencil (say, in web UI) which results in a failure in stencil eval. We want to return a nice error to the user, not crash the server.

So, I've adjusted the signature. I will (separately) change the definition of TextGenerator in Transformer.


================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:101
+    S.Parts.reserve(sizeof...(Parts));
+    auto Unused = {(S.push(std::forward<Ts>(Parts)), true)...};
+    (void)Unused;
----------------
sbenza wrote:
> We could simplify this code if you change `void push(T)` to instead `StencilPart wrap(T)`.
> Then this could be:
> 
> 
> ```
> Stencil S;
> S.Parts = {wrap(std::forward<Ts>(Parts))...};
> return S;
> ```
Much better. I believe the current design came about when StencilPart was move only. Thanks!


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78
+
+template <> bool isEqualData(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;
----------------
sbenza wrote:
> Any particular reason to use function template specialization instead of function overloading?
> The former is rare and thus harder to reason about.
> 
> (same for `evalData` below)
Stylistic -- it forces you to explicitly declare the "overload set" so to speak.  However, i'm not particularly attached -- if you think this a bad idea given its rarity, I'm happy to use overload sets. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371