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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
