sammccall created this revision.
sammccall added reviewers: ilya-biryukov, tom-anders.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang-tools-extra.
The simplest way to ensure full canonicalization is to canonicalize
recursively in most cases.
This fixes an assertion failure and presumably correctness bugs.
It does show up that D132797 <https://reviews.llvm.org/D132797>'s index-based
virtual method renames doesn't handle
templates well (the AST behavior is different and IMO better).
We could choose to disable in this case or change the index behavior,
but this patch doesn't do either.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D133415
Files:
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1515,17 +1515,21 @@
}
)cpp",
},
- // FIXME: triggers an assertion failure due to a bug in canonicalization.
- // See https://reviews.llvm.org/D132797
-#if 0
{
// virtual templated method
R"cpp(
template <typename> class Foo { virtual void [[m]](); };
class Bar : Foo<int> { void [[^m]]() override; };
- )cpp", ""
+ )cpp",
+ R"cpp(
+ #include "foo.h"
+
+ template<typename T> void Foo<T>::[[m]]() {}
+ // FIXME: not renamed as the index doesn't see this as an override of
+ // the canonical Foo<T>::m(), rather it overrides Foo<float>::m().
+ class Baz : Foo<float> { void m() override; };
+ )cpp"
},
-#endif
{
// rename on constructor and destructor.
R"cpp(
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -100,14 +100,13 @@
return canonicalRenameDecl(Method->getParent());
if (const FunctionDecl *InstantiatedMethod =
Method->getInstantiatedFromMemberFunction())
- Method = cast<CXXMethodDecl>(InstantiatedMethod);
+ return canonicalRenameDecl(InstantiatedMethod);
// FIXME(kirillbobyrev): For virtual methods with
// size_overridden_methods() > 1, this will not rename all functions it
// overrides, because this code assumes there is a single canonical
// declaration.
- while (Method->isVirtual() && Method->size_overridden_methods())
- Method = *Method->overridden_methods().begin();
- return Method->getCanonicalDecl();
+ if (Method->isVirtual() && Method->size_overridden_methods())
+ return canonicalRenameDecl(*Method->overridden_methods().begin());
}
if (const auto *Function = dyn_cast<FunctionDecl>(D))
if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
@@ -132,8 +131,7 @@
}
if (const auto *VD = dyn_cast<VarDecl>(D)) {
if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
- VD = OriginalVD;
- return VD->getCanonicalDecl();
+ return canonicalRenameDecl(OriginalVD);
}
return dyn_cast<NamedDecl>(D->getCanonicalDecl());
}
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1515,17 +1515,21 @@
}
)cpp",
},
- // FIXME: triggers an assertion failure due to a bug in canonicalization.
- // See https://reviews.llvm.org/D132797
-#if 0
{
// virtual templated method
R"cpp(
template <typename> class Foo { virtual void [[m]](); };
class Bar : Foo<int> { void [[^m]]() override; };
- )cpp", ""
+ )cpp",
+ R"cpp(
+ #include "foo.h"
+
+ template<typename T> void Foo<T>::[[m]]() {}
+ // FIXME: not renamed as the index doesn't see this as an override of
+ // the canonical Foo<T>::m(), rather it overrides Foo<float>::m().
+ class Baz : Foo<float> { void m() override; };
+ )cpp"
},
-#endif
{
// rename on constructor and destructor.
R"cpp(
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -100,14 +100,13 @@
return canonicalRenameDecl(Method->getParent());
if (const FunctionDecl *InstantiatedMethod =
Method->getInstantiatedFromMemberFunction())
- Method = cast<CXXMethodDecl>(InstantiatedMethod);
+ return canonicalRenameDecl(InstantiatedMethod);
// FIXME(kirillbobyrev): For virtual methods with
// size_overridden_methods() > 1, this will not rename all functions it
// overrides, because this code assumes there is a single canonical
// declaration.
- while (Method->isVirtual() && Method->size_overridden_methods())
- Method = *Method->overridden_methods().begin();
- return Method->getCanonicalDecl();
+ if (Method->isVirtual() && Method->size_overridden_methods())
+ return canonicalRenameDecl(*Method->overridden_methods().begin());
}
if (const auto *Function = dyn_cast<FunctionDecl>(D))
if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
@@ -132,8 +131,7 @@
}
if (const auto *VD = dyn_cast<VarDecl>(D)) {
if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
- VD = OriginalVD;
- return VD->getCanonicalDecl();
+ return canonicalRenameDecl(OriginalVD);
}
return dyn_cast<NamedDecl>(D->getCanonicalDecl());
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
