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;
----------------
Why not use `llvm::Expected<std::string>` as the return type?
================
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;
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:94
+ return false;
+ return Impl->isEqual(*(Other.Impl));
+ }
----------------
Parens around Other.Impl are not necessary.
================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:110
+ template <typename... Ts> static Stencil cat(Ts &&... Parts) {
+ Stencil Stencil;
+ Stencil.Parts.reserve(sizeof...(Parts));
----------------
I don't like naming the variable the same as the type.
You could just use `S` as the variable name. That is ok with llvm style for
small functions.
================
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;
----------------
"asserts" as it only fails in debug mode?
================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:130
+
+ bool operator==(const Stencil &Other) const {
+ return Parts == Other.Parts;
----------------
I usually prefer free functions for binary operators (friends is fine).
It makes them symmetric.
================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:155
+/// statement.
+StencilPart snode(llvm::StringRef Id);
+
----------------
`sNode` ?
ie camelCase
================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:45
+
+namespace {
+// An arbitrary fragment of code within a stencil.
----------------
maybe this anon namespace should cover the functions above?
================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:76
+
+static bool operator==(const RawTextData &A, const RawTextData &B) {
+ return A.Text == B.Text;
----------------
If you define ==, imo you should define != too.
Otherwise just call it `isEqual`
================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:94
+Error evalData(const T &Data, const MatchFinder::MatchResult &Match,
+ std::string *Result);
+
----------------
alignment.
(below too)
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
