[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

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