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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

didn't finish all parts (looked at the FunctionDecl and RecordDecl), I think we need more documentation/comments in the code to specify the canonical behavior (templates are tricky).



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+    return canonicalRenameDecl(Template);
+  return D;
----------------
the `auto` + varies `canonicalRenameDecl` overrides make the code hard to follow.

since these functions are small, I think we can inline them into the main `canonicalRenameDecl(const NamedDecl *D)`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:246
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+    return Primary;
+  return TemplatedDecl;
----------------
didn't quite follow the code here, the code looks like we get the FunctionTemplateDecl back via the code path (FunctionTemplateDecl -> FunctionDecl -> FunctionTemplateDecl), and it looks like we're not using the specialized declaration as the canonical decl.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
----------------
want to confirm my understanding:

given the example below:

```
template<typename T>
class Foo {};

template<>
class Foo<int> {};
```

the AST is like:

```
ClassTemplateDecl
  |-CXXRecordDecl (Foo definition) -> (1)
  |-ClassTemplateSpecialization. 

ClassTemplateSpecializationDecl -> call canonicalRenameDecl on it.
  |-Template Argument int
  |-CXXRecordDecl -> (2)
```

if we pass `ClassTemplateSpecializationDecl` to this function, this function will return (2)?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:261
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
----------------
I think the motivation is for merely renaming the explicit template specialization, but not the primary template?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:610
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
   if (RenameDecl.getName() == RInputs.NewName)
----------------
 `getCanonicalDecl` is not needed now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880