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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
