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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > 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.
> Up to you, to me it feels like the presence of this field defines what this class is used for.
> 1. If there's an explanation, it feels like it should represent a complete fix or refactoring that could be presented to the user.
> 2. Without an explanation, it might feel like something lower-level (e.g. one could write a bunch of RewriteRule whose changes are later combined and surfaced to the user as a full refactoring).
> 
> Both approaches make sense, and I assume (1) is the desired abstraction this class represents, so keeping the field looks ok.
Agreed. Keeping it.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > 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.
> Yeah, exactly, but could we keep is a bit shorter by removing the pieces which are non-relevant to the actual transformer usage.
> Something like:
> ```
> // To apply a rule to the clang AST, use Transformer class:
> // \code
> //   AtomicChanges Changes;
> //   Transformer T(
> //       MyRule, [&Changes](const AtomicChange &C) { Changes.push_back(C); };);
> //   ast_matchers::MatchFinder MatchFinder;
> //   T.registerMatchers(&MatchFinder);
> //   // ... insert code to run the ast matchers.
> //   // Consume the changes.
> //   applyAtomicChanges(..., Changes);
> ```
> 
> Or just mention that `Transformer` class should be used to apply the rewrite rule and obtain the corresponding replacements.
went w/ the second suggestion.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+  RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+      : Matcher(initMatcher(M)), Target(RootId), TargetKind(M.getSupportedKind()),
----------------
ilya-biryukov wrote:
> NIT: Maybe remove the constructor and let the builder handle this?
> Technically, users can break the invariants after creating the rewrite rules anyway, so having this constructor does not buy much in terms of safer coding practices, but makes it harder to use `RewriteRule` as a plain struct (specifically, having no default constructor might make it awkward).
> 
> Up to you, of course.
Agreed, but DynTypedMatcher has no default constructor, so we have to provide something.  I dropped the constructor, but added an initializer to `Matcher` to enable the default constructor.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher<T> M) {
+  return RewriteRuleBuilder(M);
+}
----------------
ilya-biryukov wrote:
> Isn't this overload redundant in presence of `buildRule(DynTypedMatcher)`? Both seem to call into the constructor that accept a `DynTypedMatcher`, so `Matcher<T>` is convertible to `DynTypedMatcher`, right?.
I think I was avoiding some extra construction, but even if so, I can't see its worth the added complexity. thx


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > 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?
> Yeah, you should explicitly qualify to let the compiler know the namespace of a corresponding declaration:
> ```
> Expected<Transformation>
> clang::tooling::applyRewriteRule(...) {
>   // ...
> }
> ```
> 
> `tooling::applyRewriteRule` would also work since we have `using namespace clang::tooling;` at the top of the file.
ah...


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+    return Transformation();
+
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > 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?
> The current approach LG, and we have the comments about those cases.
> 
> I agree with you that handling macros is hard, so skipping them makes sense for most rewrite rules one would write and at the same time, it'd be useful to let the users of the code know that a rewrite rule could not be applied because of macros.
> Returning an empty transformation in that case seems like the best option, the alternative that I can think is less nice: we could have a special error for macros and make users `llvm::handleErrors` to figure out those cases. This is clunky and the resulting code is not very nice.
> 
> 
Agreed. If error handling was nicer, then I think that your suggestion would be ideal.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:61
+)cc";
+} // namespace
+
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > 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.
> LG.
> As a note, my colleagues mentioned that the LLVM Style Guide says one should put 'static' even in that case, because it makes it easier for readers of the code to spot the file-private functions (they don't need to look for an enclosing anonymous namespace).
> 
> I'm personally happy either with or without static.
static sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376