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; ---------------- ymandel wrote: > 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? I think its fine as it is then. I don't really think the performance would matter that much, but I don't see a reason to add all those copies unnecessarily. Mention that `Result` is in an unspecified state in case of error. ================ 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; ---------------- 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. ================ 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; ---------------- 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; ``` ================ Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78 + +template <> bool isEqualData(const RawTextData &A, const RawTextData &B) { + return A.Text == B.Text; ---------------- 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) 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
