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