[PATCH] D133415: [clangd] Fix non-idempotent cases of canonicalRenameDecl()

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG5f7f4846e826: [clangd] Fix non-idempotent cases of 
canonicalRenameDecl() (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D133415?vs=458424&id=465504#toc

Repository:
  rG LLVM Github Monorepo

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

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,22 @@
         }
       )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().
+          // https://github.com/clangd/clangd/issues/1325
+          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,22 @@
         }
       )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().
+          // https://github.com/clangd/clangd/issues/1325
+          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