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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for the detailed review and really helpful comments!



================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:38
+/// @{
+using ast_matchers::CXXCtorInitializerMatcher;
+using ast_matchers::DeclarationMatcher;
----------------
ilya-biryukov wrote:
> I'm not sure if this is in the LLVM style guide, but we might want to avoid introducing these names into `clang::tooling` namespaces in the headers.
> 
> My fear is that users will rely on those using without knowing that explicitly and won't add corresponding `using` directives or qualifiers to their `.cpp` files, making refactoring and moving the code around harder.
> 
> Could you fully-qualify those names in the header instead? There does not seem to be too many of them.
right. I'd intended to introduce these into the clang tooling namespace -- that is, these weren't just convenience aliases for the header file. But, I no longer think that's useful in any case, so dropping them is certainly best.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:65
+
+using TextGenerator = std::function<llvm::Expected<std::string>(
+    const ast_matchers::MatchFinder::MatchResult &)>;
----------------
ilya-biryukov wrote:
> Why would a `TextGenerator`  fail?
> I imagine all of the failure cases are programming errors (matchers in the rewrite rule were not aligned with the corresponding text generating function). For those cases, using the `assert` macro seems cleaner.
Sure.  I could go either way. I think some of these cases fall on the border between an invariant violation and "invalid argument" or some such.  But, let's keep it simpler for now.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:116
+  // never be the empty string.
+  std::string Target = RootId;
+  ast_type_traits::ASTNodeKind TargetKind;
----------------
ilya-biryukov wrote:
> NIT: maybe move all inits to the constructor?
> To have all initializers in one place.
nicer, thanks.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:170
+
+  RewriteRuleBuilder(const RewriteRuleBuilder &) = default;
+  RewriteRuleBuilder(RewriteRuleBuilder &&) = default;
----------------
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.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:28
+
+namespace clang {
+namespace tooling {
----------------
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?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:30
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::MatchFinder;
----------------
ilya-biryukov wrote:
> Why put using directives into an anonymous namespace?
> I have not seen this pattern before, could you point me to explanations on why this is useful?
You gain an extra little bit of robustness against clashing with something declared in the enclosing namespace.  But, this is overkill even for me -- I generally only do this when I already have an anonymous namespace; there's no good reason to create one for this purpose.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:111
+// Requires verifyTarget(node, target_part) == success.
+static CharSourceRange getTarget(const DynTypedNode &Node, ASTNodeKind Kind,
+                                 NodePart TargetPart, ASTContext &Context) {
----------------
ilya-biryukov wrote:
> NIT: consider merging `verifyTarget` into `getTarget` and making `getTarget` return `Expected<CharSourceRange>`.
> Would allow avoiding to write one of the complicated switches and error-checking arguably looks just as natural in the `getTarget` as it is in `verifyTarget`.
> 
> Also, having the invariants closer to the code using them makes it easier to certify both are correct, e.g. seeing that `NamedDecl.isIdentifier()` was checked before accessing the `NamedDecl.getName()` in the same function is simpler.
Yes, I like it much better this way.  The split wasn't worth it.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:134
+      auto R = CharSourceRange::getTokenRange(TokenLoc, TokenLoc);
+      // Verify that the range covers exactly the name. FIXME: extend this code
+      // to support cases like `operator +` or `foo<int>` for which this range
----------------
ilya-biryukov wrote:
> NIT: start a fixme at a new line to make it more visible.
> Or is it `clang-format` merging it into the previous line?
> 
Done.
It's emacs (c-fill-paragraph), clang-format is better about this, because it's very conservative about touching comments.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:144
+    if (const auto *E = Node.get<clang::DeclRefExpr>()) {
+      TokenLoc = E->getLocation();
+      break;
----------------
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.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:160
+                                          const MatchResult &Result) {
+  // Ignore results in failing TUs or those rejected by the where clause.
+  if (Result.Context->getDiagnostics().hasErrorOccurred())
----------------
ilya-biryukov wrote:
> The comment still mentions the where clause. A leftover from the previous version?
right. deleted the comment altogether.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:199
+
+// `Explanation` is a `string&`, rather than a `string` or `StringRef` to save
+// an extra copy needed to intialize the captured lambda variable.  After C++14,
----------------
ilya-biryukov wrote:
> Maybe use `std::string` in the public interface anyway? 
> - This function is definitely not a bottleneck, so an extra copy is not a big deal for performance.
> - Using `std::string` would let the clients save a copy in their client code if they can (by using `std::move` or creating an r-value string in the first place).
> - We will have a copy in the body of our function (we have it anyway). We'll eliminate it after switching to C++14 in the body of our function without changing the clients.
Agreed. Updated all relevant places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376