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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+    return canonicalRenameDecl(Template);
+  return D;
----------------
hokein wrote:
> 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)`
I removed `auto`s but I believe merging them into a single function would not be great for two reasons:

* Some classes are already handled by the same function outside of the main one: ctor/dtor's parent `CXXRecordDecl`, etc: in the main function we'd have to have a weird logic behind extracting those and this is likely to result in more code
* Some functions call other ones (e.g. here we have `canonicalRenameDecl(Template)` - sure, right now it's just `D->getTemplatedDecl()` but if we decide to change something and when we introduce more code it'd be easy to forget and code duplication is not really a good idea).

WDYT?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:246
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+    return Primary;
+  return TemplatedDecl;
----------------
hokein wrote:
> 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.
Uh, good catch, thanks. This one is simply not needed because we handle `TemplateDecl` which does the right thing for `FunctionTemplateDecl`.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
----------------
hokein wrote:
> 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)?
No, this will return (1): `getSpecializedTemplate()` returns `ClassTemplateDecl` and then `getTemplatedDecl()` gets to `CXXRecordDecl` in it.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71880