[clangd] Let DefineOutline tweak handle member function templates #112345

ckandeler · 2024-10-15T11:04:04Z

No description provided.

llvmbot · 2024-10-15T11:04:39Z

@llvm/pr-subscribers-clangd

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

Author: Christian Kandeler (ckandeler)

Changes

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

2 Files Affected:

(modified) clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp (+50-19)
(modified) clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp (+44-7)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index 591a8b245260ea..14d9858b702a1d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -109,7 +109,8 @@ findContextForNS(llvm::StringRef TargetNS, const DeclContext *CurContext) {
 // afterwards it can be shared with define-inline code action.
 llvm::Expected<std::string>
 getFunctionSourceAfterReplacements(const FunctionDecl *FD,
-                                   const tooling::Replacements &Replacements) {
+                                   const tooling::Replacements &Replacements,
+                                   bool TargetFileIsHeader) {
   const auto &SM = FD->getASTContext().getSourceManager();
   auto OrigFuncRange = toHalfOpenFileRange(
       SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());
@@ -130,23 +131,41 @@ getFunctionSourceAfterReplacements(const FunctionDecl *FD,
     return QualifiedFunc.takeError();
 
   std::string TemplatePrefix;
+  auto AddToTemplatePrefixIfApplicable = [&](const Decl *D, bool append) {
+    const TemplateParameterList *Params = D->getDescribedTemplateParams();
+    if (!Params)
+      return;
+    for (Decl *P : *Params) {
+      if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(P)) {
+        TTP->removeDefaultArgument();
+      } else if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(P)) {
+        NTTP->removeDefaultArgument();
+      } else if (auto * TTPD = dyn_cast<TemplateTemplateParmDecl>(P)) {
+        TTPD->removeDefaultArgument();
+      }
+    }
+    std::string S;
+    llvm::raw_string_ostream Stream(S);
+    Params->print(Stream, FD->getASTContext());
+    if (!S.empty())
+      *S.rbegin() = '\n'; // Replace space with newline
+    if (append)
+      TemplatePrefix.append(S);
+    else
+      TemplatePrefix.insert(0, S);
+  };
   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 (const TemplateParameterList *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);
-      }
+      AddToTemplatePrefixIfApplicable(Parent, false);
     }
   }
-
+  
+  AddToTemplatePrefixIfApplicable(FD, true);
   auto Source = QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+  if (TargetFileIsHeader)
+    Source.insert(0, "inline ");
   if (!TemplatePrefix.empty())
     Source.insert(0, TemplatePrefix);
   return Source;
@@ -202,7 +221,8 @@ deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
 llvm::Expected<std::string>
 getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
                       const syntax::TokenBuffer &TokBuf,
-                      const HeuristicResolver *Resolver) {
+                      const HeuristicResolver *Resolver,
+                      bool targetFileIsHeader) {
   auto &AST = FD->getASTContext();
   auto &SM = AST.getSourceManager();
 
@@ -337,7 +357,7 @@ getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
 
   if (Errors)
     return std::move(Errors);
-  return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
+  return getFunctionSourceAfterReplacements(FD, DeclarationCleanups, targetFileIsHeader);
 }
 
 struct InsertionPoint {
@@ -419,15 +439,15 @@ class DefineOutline : public Tweak {
         Source->isOutOfLine())
       return false;
 
-    // Bail out if this is a function template or specialization, as their
+    // Bail out if this is a function template specialization, as their
     // definitions need to be visible in all including translation units.
-    if (Source->getDescribedFunctionTemplate())
-      return false;
     if (Source->getTemplateSpecializationInfo())
       return false;
 
     auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source);
     if (!MD) {
+      if (Source->getDescribedFunctionTemplate())
+        return false;
       // Can't outline free-standing functions in the same file.
       return !SameFile;
     }
@@ -443,13 +463,24 @@ class DefineOutline : public Tweak {
         SameFile = true;
 
         // Bail out if the template parameter is unnamed.
+        // FIXME: Is this really needed? It inhibits application on
+        //        e.g. std::enable_if.
         for (NamedDecl *P : *Params) {
           if (!P->getIdentifier())
             return false;
         }
       }
     }
-
+    
+    // For function templates, the same limitations as for class templates apply.
+    if (const TemplateParameterList *Params = MD->getDescribedTemplateParams()) {
+      for (NamedDecl *P : *Params) {
+        if (!P->getIdentifier())
+          return false;
+      }
+      SameFile = true;
+    }
+        
     // The refactoring is meaningless for unnamed classes and namespaces,
     // unless we're outlining in the same file
     for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
@@ -459,7 +490,7 @@ class DefineOutline : public Tweak {
           return false;
       }
     }
-
+    
     // Note that we don't check whether an implementation file exists or not in
     // the prepare, since performing disk IO on each prepare request might be
     // expensive.
@@ -485,7 +516,7 @@ class DefineOutline : public Tweak {
 
     auto FuncDef = getFunctionSourceCode(
         Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens(),
-        Sel.AST->getHeuristicResolver());
+        Sel.AST->getHeuristicResolver(), SameFile && isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts()));
     if (!FuncDef)
       return FuncDef.takeError();
 
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index 6a9e90c3bfa70f..7e2b863733477d 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -111,13 +111,17 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
     template <typename> struct Foo { void fo^o(){} };
     )cpp");
 
-  // Not available on function templates and specializations, as definition must
-  // be visible to all translation units.
+  // Not available on function template specializations and free function templates.
   EXPECT_UNAVAILABLE(R"cpp(
-    template <typename> void fo^o() {};
-    template <> void fo^o<int>() {};
+    template <typename T> void fo^o() {}
+    template <> void fo^o<int>() {}
   )cpp");
-
+  
+  // Not available on member function templates with unnamed template parameters.
+  EXPECT_UNAVAILABLE(R"cpp(
+    struct Foo { template <typename> void ba^r() {} };
+  )cpp");
+  
   // Not available on methods of unnamed classes.
   EXPECT_UNAVAILABLE(R"cpp(
     struct Foo {
@@ -237,7 +241,7 @@ TEST_F(DefineOutlineTest, ApplyTest) {
                 Foo(T z) __attribute__((weak)) ;
                 int bar;
               };template <typename T>
-Foo<T>::Foo(T z) __attribute__((weak)) : bar(2){}
+inline Foo<T>::Foo(T z) __attribute__((weak)) : bar(2){}
 )cpp",
           ""},
       // Virt specifiers.
@@ -390,7 +394,7 @@ Foo<T>::Foo(T z) __attribute__((weak)) : bar(2){}
               };
             };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; }
+inline 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
@@ -399,6 +403,39 @@ typename O1<T, U...>::template O2<V, A>::E O1<T, U...>::template O2<V, A>::I::fo
           "class A { ~A(); };",
           "A::~A(){} ",
       },
+      
+      // Member template
+      {
+          R"cpp(
+            struct Foo {
+              template <typename T, bool B = true>
+              void ^bar() {}
+            };)cpp",
+          R"cpp(
+            struct Foo {
+              template <typename T, bool B = true>
+              void bar() ;
+            };template <typename T, bool B>
+inline void Foo::bar() {}
+)cpp",
+          ""
+      },
+      
+      // Class template with member template
+      {
+          R"cpp(
+            template <typename T> struct Foo {
+              template <typename U> void ^bar(const T& t, const U& u) {}
+            };)cpp",
+          R"cpp(
+            template <typename T> struct Foo {
+              template <typename U> void bar(const T& t, const U& u) ;
+            };template <typename T>
+template <typename U>
+inline void Foo<T>::bar(const T& t, const U& u) {}
+)cpp",
+          ""
+      },
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);

HighCommander4 · 2024-10-18T04:44:25Z

It looks like it was a deliberate design choice to disable this tweak for templates: https://reviews.llvm.org/D85310.

cc @kadircet, @hokein for any thoughts

kadircet · 2024-10-18T07:56:13Z

i remember mainly two concerns;

we didn't want to surprise developers by breaking their builds as template definitions needs to be visible at instantiation sites. so if we keep the definition in the same file, just move it out-of-line, this should be fine.
i remember AST was lacking information about template parameter names, maybe that's resolved now. This was causing issues as you need to print the whole outer template parameter list as-is, e.g. in the tests behavior looks like:

template <typename U>
inline void Foo<T>::bar(const T& t, const U& u) {}

this won't compile as T isn't defined. you also need to print template <typename T> on top (and keep doing for rest of the outer decls).

if we can address both of these, i don't think there's any other reason to not have them

ckandeler · 2024-10-18T09:03:47Z

template <typename U>
inline void Foo<T>::bar(const T& t, const U& u) {}
this won't compile as T isn't defined. you also need to print template <typename T> on top (and keep doing for rest of the outer decls).

The patch does that already.
In the test, the outer template declaration is on the line above.

HighCommander4

LGTM with minor comments addressed

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

HighCommander4 · 2024-11-18T03:55:15Z

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

@@ -443,13 +461,26 @@ class DefineOutline : public Tweak {
        SameFile = true;

        // Bail out if the template parameter is unnamed.
+        // FIXME: Is this really needed? It inhibits application on


Reviewing the discussion here, the reason is that we have to mention the template parameter as part of the out-of-line declaration, e.g.:

Before:

template <typename T, typename> struct A { void foo() {} };

After:

template <typename T, typename> struct A { void foo(); }; template <typename T, typename> void A<T, ???>::foo() {}

I've moved the comment down to the function template part.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

HighCommander4 · 2024-11-19T06:27:04Z

General process comment: when force-pushing, it would help me as a reviewer to push any rebase separately from the actual patch update.

When the two are combined, there is no way (that I've found) for me to get a view of just the changes since the last version.

HighCommander4

Anyways, LGTM!

llvmbot added clang-tools-extra clangd labels Oct 15, 2024

ckandeler force-pushed the define-outline-for-member-templates branch 3 times, most recently from 5b729d0 to 42ea89f Compare October 15, 2024 12:11

ckandeler requested a review from HighCommander4 October 15, 2024 13:08

HighCommander4 approved these changes Nov 18, 2024

View reviewed changes

[clangd] Let DefineOutline tweak handle member function templates

5b563ce

ckandeler force-pushed the define-outline-for-member-templates branch from 42ea89f to 5b563ce Compare November 18, 2024 12:10

HighCommander4 approved these changes Nov 19, 2024

View reviewed changes

ckandeler merged commit 8a6a76b into llvm:main Nov 19, 2024
8 checks passed

ckandeler deleted the define-outline-for-member-templates branch November 19, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clangd] Let DefineOutline tweak handle member function templates #112345

[clangd] Let DefineOutline tweak handle member function templates #112345

Uh oh!

ckandeler commented Oct 15, 2024

Uh oh!

llvmbot commented Oct 15, 2024 •

edited

Loading

Uh oh!

HighCommander4 commented Oct 18, 2024

Uh oh!

kadircet commented Oct 18, 2024

Uh oh!

ckandeler commented Oct 18, 2024

Uh oh!

HighCommander4 left a comment

Uh oh!

Uh oh!

HighCommander4 Nov 18, 2024

Uh oh!

ckandeler Nov 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HighCommander4 commented Nov 19, 2024

Uh oh!

HighCommander4 left a comment

Uh oh!

Uh oh!

Uh oh!

[clangd] Let DefineOutline tweak handle member function templates #112345

[clangd] Let DefineOutline tweak handle member function templates #112345

Uh oh!

Conversation

ckandeler commented Oct 15, 2024

Uh oh!

llvmbot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HighCommander4 commented Oct 18, 2024

Uh oh!

kadircet commented Oct 18, 2024

Uh oh!

ckandeler commented Oct 18, 2024

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HighCommander4 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

ckandeler Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HighCommander4 commented Nov 19, 2024

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Oct 15, 2024 •

edited

Loading