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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274
+    return canonicalRenameDecl(Function);
+  return D;
+}
----------------
I think we should call `getCanonicalDecl` before returning the final result (calling it at the beginning doesn't guarantee the final result is canonical).


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:601
     return makeError(ReasonToReject::AmbiguousSymbol);
-  const auto &RenameDecl =
-      llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  const auto &RenameDecl = llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin()));
   if (RenameDecl.getName() == RInputs.NewName)
----------------
nit: llvm::cast is not needed, just `const NamedDecl &RenameDecl = *(*DeclsUnderCursor.begin());`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+    return canonicalRenameDecl(Template);
+  return D;
----------------
kbobyrev wrote:
> 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?
this is more about encapsulation -- my understanding is that `canonicalRenameDecl(const NamedDecl)` should be the only main entry, others are just implementation details, so we expect client just calls the main function, but not other overrides.  The current state leaks these implementation details outside of the world.


> 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

don't follow this, for ctor/dtor's parent CXXRecordDecl, I think just calling the main canonicalRenameDecl recursively on the parent should be enough?


> 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).

I agree your points. Given the current code, I think inlining them seems quite straight-forward and clearer to me (the only case is `FunctionDecl`, which calls the `canonicalRenameDecl(const TemplateDecl *D)`, but we can just call `getTemplatedDecl` as well). 

Regarding your concerns, I think we can always find ways to extend the code when we need to enhance the TemplateDecl case (what kind of enhancement we'll do for templateDecl?)



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
----------------
kbobyrev wrote:
> 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.
I see. thanks for the explanation. I misunderstood the `getSpecializedTemplate`.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:261
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
----------------
kbobyrev wrote:
> 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?
ah, you're right, sorry -- I was confused by the term "specialized template", I thought it is the specialization decl. 

I'd suggest using "primary template", I think it is clearer than the `specialized decl.`, and would be nice to have some concrete example in comments explaining these tricky cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71880