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