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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

The only real question I have is about returning an error vs an empty transformation in of macros.
The rest are just NITs.

Thanks for the change! I'll get to the NodeId patch right away :-)



================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:170
+
+  RewriteRuleBuilder(const RewriteRuleBuilder &) = default;
+  RewriteRuleBuilder(RewriteRuleBuilder &&) = default;
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > NIT: maybe remove the `=default` copy and move ctors and assignments?
> > They should be generated automatically anyway, right?
> Sure. I was going based on google's style recommendations, but personally i prefer leaving them implicit.
+1. Means less boilterplate.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//
----------------
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?


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode
----------------
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.


================
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.
----------------
NIT: maybe assert this invariant in the constructor?


================
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; }
+
----------------
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?


================
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))));
+  }
----------------
NIT: the top-level `std::move` looks redundant, maybe remove it?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:31
+
+using ::clang::ast_matchers::MatchFinder;
+using ::clang::ast_type_traits::ASTNodeKind;
----------------
NIT: we could leave out the trailing `::` for `clang` and `llvm`, they are well-known and unambiguous namespace names.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:42
+
+static bool isOriginMacroBody(const clang::SourceManager &SM,
+                              clang::SourceLocation Loc) {
----------------
NIT: could you add a brief comment on what this function is expected to return?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {
----------------
NIT: remove namespaces for consistency with the rest of the code.

(Probably a leftover from the previous version)


================
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());
----------------
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.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+    return Transformation();
+
----------------
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.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:28
+
+namespace clang {
+namespace tooling {
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > Other files in the tooling library seem to be adding `using namespace clang` instead of putting the declaration into a namespace.
> > Could you please change the new code to do the same for consistency?
> > 
> Done.  Agreed about being consistent. FWIW, I can't say I like this style.  Perhaps because I'm not used to it, but it feels too implicit.  It forces the reader to figure out where each definition is being associated. Also, I discovered it only works for method definitions. Free functions still need to be explicitly namespaced.
> 
> Any idea what the reason for this style is?
I personally have a slight preference towards `using namespace` style myself because it produces compiler errors whenever declaration and definition signatures don't match, catching errors early in cases where one forgets to update either of those, e.g.
```

// foo.h
namespace clang{
std::unique_ptr<Something> createSomething();
}

// foo1.cpp
using namespace clang;

std::unique_ptr<Something> clang::createSomething(int Param) { // <-- error
}


// foo2.cpp
namespace clang {
std::unique_ptr<Something> clang::createSomething(int Param) { // <-- all ok, will get linker errors on usages instead.
}
}
```

There might be other reasons and I'm not sure why exactly the code in clang uses this approach.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:144
+    if (const auto *E = Node.get<clang::DeclRefExpr>()) {
+      TokenLoc = E->getLocation();
+      break;
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > This could be `operator+` too, right?
> > ```
> > struct X {
> >   X operator+(int);
> > 
> >   void test() {
> >     X (X::*ptr)(int) = &X::operator+;
> >   }
> > };
> > ```
> > 
> > Would probably want to bail out in this case as well.
> The check `E->getNameInfo().getName().isIdentifier()` rules that out.  It was hidden in `verifyTarget()`. I hope the new code makes this clearer?
> Also, I updated the test `TransformerTest.NodePartNameDeclRefFailure` to cover this case.
Right, thanks! Moving the check into the code makes it more explicit now, thanks!


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:61
+)cc";
+} // namespace
+
----------------
Maybe put the whole file into an anonymous namespace?
To avoid potential name clashes with other test files (and ODR violations that might follow)


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:88
+  if (I != std::string::npos) {
+    Actual.erase(I, HL.size());
+  }
----------------
NIT: remove redundant braces


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:159
+
+  Transformer T(ruleStrlenSize(), changeRecorder());
+  T.registerMatchers(&MatchFinder);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376