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