[clangd] Let DefineOutline tweak handle member functions

[ckandeler]

... of class templates.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Christian Kandeler (ckandeler)

Changes

... of class templates.

---

Full diff: https://github.com/llvm/llvm-project/pull/95235.diff

3 Files Affected:

• (modified) clang-tools-extra/clangd/AST.cpp (+6-1)
• (modified) clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp (+41-6)
• (modified) clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp (+42-13)

diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index fda1e5fdf8d82..2f2f8437d6ed9 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -144,8 +144,13 @@ getQualification(ASTContext &Context, const DeclContext *DestContext,
   // since we stored inner-most parent first.
   std::string Result;
   llvm::raw_string_ostream OS(Result);
-  for (const auto *Parent : llvm::reverse(Parents))
+  for (const auto *Parent : llvm::reverse(Parents)) {
+    if (Parent != *Parents.rbegin() && Parent->isDependent() &&
+        Parent->getAsRecordDecl() &&
+        Parent->getAsRecordDecl()->getDescribedClassTemplate())
+      OS << "template ";
     Parent->print(OS, Context.getPrintingPolicy());
+  }
   return OS.str();
 }
 
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index f43f2417df8fc..15cb586e6c21e 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -128,7 +128,27 @@ getFunctionSourceAfterReplacements(const FunctionDecl *FD,
       SM.getBufferData(SM.getMainFileID()), Replacements);
   if (!QualifiedFunc)
     return QualifiedFunc.takeError();
-  return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+
+  std::string TemplatePrefix;
+  if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(FD)) {
+    for (const CXXRecordDecl *Parent = MD->getParent(); Parent;
+         Parent =
+             llvm::dyn_cast_or_null<const CXXRecordDecl>(Parent->getParent())) {
+      if (auto Params = Parent->getDescribedTemplateParams()) {
+        std::string S;
+        llvm::raw_string_ostream Stream(S);
+        Params->print(Stream, FD->getASTContext());
+        if (!S.empty())
+          *S.rbegin() = '\n'; // Replace space with newline
+        TemplatePrefix.insert(0, S);
+      }
+    }
+  }
+
+  auto Source = QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+  if (!TemplatePrefix.empty())
+    Source.insert(0, TemplatePrefix);
+  return Source;
 }
 
 // Returns replacements to delete tokens with kind `Kind` in the range
@@ -212,9 +232,13 @@ getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
           }
         }
         const NamedDecl *ND = Ref.Targets.front();
-        const std::string Qualifier =
+        std::string Qualifier =
             getQualification(AST, TargetContext,
                              SM.getLocForStartOfFile(SM.getMainFileID()), ND);
+        if (ND->getDeclContext()->isDependentContext()) {
+          if (llvm::isa<TypeDecl>(ND))
+            Qualifier.insert(0, "typename ");
+        }
         if (auto Err = DeclarationCleanups.add(
                 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
           Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -407,10 +431,21 @@ class DefineOutline : public Tweak {
       return !SameFile;
     }
 
-    // Bail out in templated classes, as it is hard to spell the class name,
-    // i.e if the template parameter is unnamed.
-    if (MD->getParent()->isTemplated())
-      return false;
+    for (const CXXRecordDecl *Parent = MD->getParent(); Parent;
+         Parent =
+             llvm::dyn_cast_or_null<const CXXRecordDecl>(Parent->getParent())) {
+      if (auto Params = Parent->getDescribedTemplateParams()) {
+
+        // Class template member functions must be defined in the
+        // same file.
+        SameFile = true;
+
+        for (NamedDecl *P : *Params) {
+          if (!P->getIdentifier())
+            return false;
+        }
+      }
+    }
 
     // The refactoring is meaningless for unnamed classes and namespaces,
     // unless we're outlining in the same file
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index 906ff33db8734..6a9e90c3bfa70 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -105,8 +105,8 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
       F^oo(const Foo&) = delete;
     };)cpp");
 
-  // Not available within templated classes, as it is hard to spell class name
-  // out-of-line in such cases.
+  // Not available within templated classes with unnamed parameters, as it is
+  // hard to spell class name out-of-line in such cases.
   EXPECT_UNAVAILABLE(R"cpp(
     template <typename> struct Foo { void fo^o(){} };
     )cpp");
@@ -154,7 +154,6 @@ TEST_F(DefineOutlineTest, FailsWithoutSource) {
 }
 
 TEST_F(DefineOutlineTest, ApplyTest) {
-  llvm::StringMap<std::string> EditedFiles;
   ExtraFiles["Test.cpp"] = "";
   FileName = "Test.hpp";
 
@@ -229,17 +228,18 @@ TEST_F(DefineOutlineTest, ApplyTest) {
       // Ctor initializer with attribute.
       {
           R"cpp(
-              class Foo {
-                F^oo(int z) __attribute__((weak)) : bar(2){}
+              template <typename T> class Foo {
+                F^oo(T z) __attribute__((weak)) : bar(2){}
                 int bar;
               };)cpp",
           R"cpp(
-              class Foo {
-                Foo(int z) __attribute__((weak)) ;
+              template <typename T> class Foo {
+                Foo(T z) __attribute__((weak)) ;
                 int bar;
-              };)cpp",
-          "Foo::Foo(int z) __attribute__((weak)) : bar(2){}\n",
-      },
+              };template <typename T>
+Foo<T>::Foo(T z) __attribute__((weak)) : bar(2){}
+)cpp",
+          ""},
       // Virt specifiers.
       {
           R"cpp(
@@ -369,7 +369,31 @@ TEST_F(DefineOutlineTest, ApplyTest) {
             };)cpp",
           " void A::foo(int) {}\n",
       },
-      // Destrctors
+      // Complex class template
+      {
+          R"cpp(
+            template <typename T, typename ...U> struct O1 {
+              template <class V, int A> struct O2 {
+                enum E { E1, E2 };
+                struct I {
+                  E f^oo(T, U..., V, E) { return E1; }
+                };
+              };
+            };)cpp",
+          R"cpp(
+            template <typename T, typename ...U> struct O1 {
+              template <class V, int A> struct O2 {
+                enum E { E1, E2 };
+                struct I {
+                  E foo(T, U..., V, E) ;
+                };
+              };
+            };template <typename T, typename ...U>
+template <class V, int A>
+typename O1<T, U...>::template O2<V, A>::E O1<T, U...>::template O2<V, A>::I::foo(T, U..., V, E) { return E1; }
+)cpp",
+          ""},
+      // Destructors
       {
           "class A { ~A^(){} };",
           "class A { ~A(); };",
@@ -378,9 +402,14 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
+    llvm::StringMap<std::string> EditedFiles;
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
-    EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
-                                 testPath("Test.cpp"), Case.ExpectedSource)));
+    if (Case.ExpectedSource.empty()) {
+      EXPECT_TRUE(EditedFiles.empty());
+    } else {
+      EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+                                   testPath("Test.cpp"), Case.ExpectedSource)));
+    }
   }
 }
 

[5chmidti]

From a functionality standpoint, this looks good to me. I noted some small things in the code, and this is also missing a release not entry.

I'll try to test this some more locally in a project.

[ckandeler]

ping

[HighCommander4]

@5chmidti since you've already looked at this, I'd be happy for you to approve this if you're comfortable doing so.

[5chmidti]

@5chmidti since you've already looked at this, I'd be happy for you to approve this if you're comfortable doing so.

Sure

[ckandeler]

Thanks!