[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.
Samuel Benzaquen via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 17 09:10:43 PDT 2019
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
More information about the cfe-commits
mailing list