hokein added a comment.
Thanks, this looks nicer now.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:247
+ if (const auto *Destructor = dyn_cast<CXXDestructorDecl>(D))
+ return canonicalRenameDecl(Destructor->getParent());
+ if (const auto *VarTemplate = dyn_cast<VarTemplateSpecializationDecl>(D))
----------------
ctor & dtor is one of the kinds of `CXXMethodDecl`, I think we can merge the
ctor & dtor cases into the `if (const auto *Method =
dyn_cast<CXXMethodDecl>(D)) {`.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:257
+ ->getTemplatedDecl();
+ if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
+ if (const FunctionDecl *InstantiatedMethod =
----------------
using `else if` would be more clear -- as now we modify `D` is the preceding
`if`, if the D is assigned to a CXXMethodDecl in the `preceding` if, then it
may fall into this branch.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:262
+ while (Method->isVirtual() && Method->size_overridden_methods())
+ Method = *Method->overridden_methods().begin();
+ D = Method;
----------------
oh, it is a tricky case where a method overrides > 1 virtual methods. Looks
like we will regress this case in this patch, e.g.
```
class Foo {
virtual void foo();
};
class Bar {
virtual void foo();
};
class C : public Foo, Bar {
void foo() override; // -> rename foo => bar
};
```
prior to the patch, both `Foo::foo` and `Bar::foo` will be renamed, after this
patch, only one of them will be renamed (with within-file rename)? I think it
is ok, but probably need some comments here.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:265
+ }
+ if (const auto *Function = dyn_cast<FunctionDecl>(D))
+ if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
----------------
the same, `else`
IIUC, `CXXMethodDecl` must be processed before `FunctionDecl`, I think we can
add an assertion (the FunctionDecl must not be `CXXMethodDecl`).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71880/new/
https://reviews.llvm.org/D71880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
