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