ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do
with the actual design of the library, so expect LGTM in the next round.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+// RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:82
+// Parameters can be declared explicitly using the NodeId type and its
+// derivatives or left implicit by using the native support for binding ids in
+// the clang matchers.
----------------
NIT: Maybe be more specific: **string-based** binding ids?
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:125
+ // Id used as the default target of each match. The node described by the
+ // matcher is guaranteed to be bound this id, for all rewrite rules.
+ static constexpr char RootId[] = "___root___";
----------------
NIT: bound **to** this id
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+ RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+ : Matcher(initMatcher(M)), Target(RootId),
TargetKind(M.getSupportedKind()),
----------------
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.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:158
+ template <typename T>
+ RewriteRuleBuilder &&change(const TypedNodeId<T> &Target,
+ NodePart Part = NodePart::Node) &&;
----------------
NIT: return by value to make the signatures less clunky. RVO should insure the
generated code is the same in practice.
Bonus point: that'd mean we don't need `&&` qualifier on the builder functions
too, only at `consume()`.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher<T> M) {
+ return RewriteRuleBuilder(M);
+}
----------------
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?.
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {
----------------
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.
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+ isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+ return Transformation();
+
----------------
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.
================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:61
+)cc";
+} // namespace
+
----------------
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.
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
