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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits