[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 19 inline comments as done.
ymandel added a comment.

Thanks for (more) helpful comments.  I think the code is a lot better now than it started out. :)

Also, converted `RewriteRule` to a simple struct as per our discussion.



================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//
----------------
ilya-biryukov wrote:
> NIT: maybe mention what it should be used for? we plan to show to to the user (e.g. in the clang-tidy fix description), right?
Done. But, given that we don't use this field yet, I'm fine deleting it until a future revision. I'm also fine leaving it given that we know we'll be needing it later.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode
----------------
ilya-biryukov wrote:
> Could you also add examples on how to apply the rewrite rule here?
> So that the users can have an idea about the full workflow when reading the code.
Is this what you had in mind? Unlike clang-tidy, there is no neat infrastructure into which we can drop it.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:97
+  ast_matchers::internal::DynTypedMatcher Matcher;
+  // The (bound) id of the node whose source will be replaced.  This id should
+  // never be the empty string.
----------------
ilya-biryukov wrote:
> NIT: maybe assert this invariant in the constructor?
The constructor sets this field, so no need to assert.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:122
+  // The bound id of the node corresponding to the matcher.
+  static llvm::StringRef matchedNode() { return RootId; }
+
----------------
ilya-biryukov wrote:
> This method does not seem to be used anywhere.
> Are we missing the usages in this patch? Maybe remove it from the initial implementation and re-add later when we have the callsites?
It was used above in makeMatcher.  But, it was overkill -- users can just reference RootId directly, so I've removed it.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:175
+  RewriteRuleBuilder &&because(std::string Explanation) && {
+    return std::move(std::move(*this).because(text(std::move(Explanation))));
+  }
----------------
ilya-biryukov wrote:
> NIT: the top-level `std::move` looks redundant, maybe remove it?
Yes, not sure why did that. thanks.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {
----------------
ilya-biryukov wrote:
> NIT: remove namespaces for consistency with the rest of the code.
> 
> (Probably a leftover from the previous version)
This is necessary as far as I can tell. W/o it, I get a linker failure (missing definition). Based on the declaration in the header, the compiler resolves the reference below to clang::tooling::applyRewriteRule() but this definition ends up in the global namespace.

I think the using decls only work for method definitions -- the type seems to resolve to be the one in the namespace. But free functions don't get the same treatment. Unless I"m doing something wrong?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:150
+          llvm::handleErrors(TargetOrErr.takeError(), [&Rule](StringError &E) {
+            return invalidArgumentError("Failure targeting node" +
+                                        Rule.target() + ": " + E.getMessage());
----------------
ilya-biryukov wrote:
> NIT: consider simply propagating the original error up the stack in that case to avoid boilerplate.
> Although adding the `.target()` information to the error might be useful, so up to you.
integrated id into getTarget so we can avoid this error handling step.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+    return Transformation();
+
----------------
ilya-biryukov wrote:
> Why not return an error in case of macros too? Is there any use of the empty transformation to the clients? 
> Alternatively, we might want to document this behavior (applyRewriteRule can return empty transformations) and it's rationale.
I think the common case is that we simply want to skip macros -- there's nothing erroneous about them, they're just usually hard to deal w/ correctly.  That said, we might want to change this to *never* return an error and just assert when things go wrong. The advantage of the current design is that callers can in principle ignore failed rewrites.

However, in practice, if the rewrite fails that means it had a bug, so it might be best to asssert().  Only real advantage, then, is that this is easier to test since it doesn't crash the program.

WDYT?


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:61
+)cc";
+} // namespace
+
----------------
ilya-biryukov wrote:
> Maybe put the whole file into an anonymous namespace?
> To avoid potential name clashes with other test files (and ODR violations that might follow)
also removed `static` qualifiers from functions since they are now redundant.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:159
+
+  Transformer T(ruleStrlenSize(), changeRecorder());
+  T.registerMatchers(&MatchFinder);
----------------
ilya-biryukov wrote:
> These three lines are in each test. Consider moving them to a helper function (e.g. returning the final result as text?) to reduce the boilerplate in the test.
Thanks, much cleaner this way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376