[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

https://github.com/ckandeler closed 
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

ckandeler wrote:

I'm going to merge this without explicit format approval based on the following:
  - There was a general LGTM.
  - I addressed the remaining issues.
  - There was no further feedback for more than half a year.

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

ckandeler wrote:

ping

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

ckandeler wrote:

ping

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

ckandeler wrote:

ping

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

ckandeler wrote:

ping

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Aart Bik via cfe-commits]

https://github.com/aartbik updated 
https://github.com/llvm/llvm-project/pull/69704

>From 40df0527b2a3af8012f32d771a1bb2c861d42ed3 Mon Sep 17 00:00:00 2001
From: Christian Kandeler 
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
 files

Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
 .../clangd/refactor/tweaks/DefineOutline.cpp  | 150 +++---
 .../unittests/tweaks/DefineOutlineTests.cpp   |  73 -
 2 files changed, 164 insertions(+), 59 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..98cb3a8770c696d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer , 
tok::TokenKind Kind,
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
   const syntax::TokenBuffer ,
   const HeuristicResolver *Resolver) {
   auto  = FD->getASTContext();
   auto  = AST.getSourceManager();
-  auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
-  if (!TargetContext)
-return error("define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 const NamedDecl *ND = Ref.Targets.front();
 const std::string Qualifier =
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()), ND);
 if (auto Err = DeclarationCleanups.add(
 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
   if (const auto *Destructor = llvm::dyn_cast(FD)) {
 if (auto Err = DeclarationCleanups.add(tooling::Replacement(
 SM, Destructor->getLocation(), 0,
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()),
  Destructor
   Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 
 struct InsertionPoint {
-  std::string EnclosingNamespace;
+  const DeclContext *EnclosingNamespace = nullptr;
   size_t Offset;
 };
-// Returns the most natural insertion point for \p QualifiedName in \p 
Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected getInsertionPoint(llvm::StringRef Contents,
- llvm::StringRef QualifiedName,
- const LangOptions ) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
-  assert(!Region.EligiblePoints.empty());
-  // FIXME: This selection can be made smarter by looking at the definition
-  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
-  // getEligibleRegions only knows about namespace begin/end events so we
-  // can't match function start/end positions yet.
-  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
-  if (!Offset)
-return Offset.takeError();
-  return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
 
 // Returns the range that should be deleted from declaration, which always
 // contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not a 

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

https://github.com/ckandeler updated 
https://github.com/llvm/llvm-project/pull/69704

>From 40df0527b2a3af8012f32d771a1bb2c861d42ed3 Mon Sep 17 00:00:00 2001
From: Christian Kandeler 
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
 files

Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
 .../clangd/refactor/tweaks/DefineOutline.cpp  | 150 +++---
 .../unittests/tweaks/DefineOutlineTests.cpp   |  73 -
 2 files changed, 164 insertions(+), 59 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..98cb3a8770c696d 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer , 
tok::TokenKind Kind,
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
   const syntax::TokenBuffer ,
   const HeuristicResolver *Resolver) {
   auto  = FD->getASTContext();
   auto  = AST.getSourceManager();
-  auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
-  if (!TargetContext)
-return error("define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 const NamedDecl *ND = Ref.Targets.front();
 const std::string Qualifier =
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()), ND);
 if (auto Err = DeclarationCleanups.add(
 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
   if (const auto *Destructor = llvm::dyn_cast(FD)) {
 if (auto Err = DeclarationCleanups.add(tooling::Replacement(
 SM, Destructor->getLocation(), 0,
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()),
  Destructor
   Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 
 struct InsertionPoint {
-  std::string EnclosingNamespace;
+  const DeclContext *EnclosingNamespace = nullptr;
   size_t Offset;
 };
-// Returns the most natural insertion point for \p QualifiedName in \p 
Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected getInsertionPoint(llvm::StringRef Contents,
- llvm::StringRef QualifiedName,
- const LangOptions ) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
-  assert(!Region.EligiblePoints.empty());
-  // FIXME: This selection can be made smarter by looking at the definition
-  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
-  // getEligibleRegions only knows about namespace begin/end events so we
-  // can't match function start/end positions yet.
-  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
-  if (!Offset)
-return Offset.takeError();
-  return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
 
 // Returns the range that should be deleted from declaration, which always
 // contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not 

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

@@ -349,6 +358,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace foo {
+namespace {
+struct Foo { void ba^r() {} };

ckandeler wrote:

Hm, the SourceLocations are weird: getEndLoc() points to the closing brace of 
the class. Do I really have to look for the semicolon manually?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

@@ -349,6 +358,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace foo {
+namespace {
+struct Foo { void ba^r() {} };

ckandeler wrote:

Ah, maybe getOuterLexicalRecordContext().

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

@@ -349,6 +358,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace foo {
+namespace {
+struct Foo { void ba^r() {} };

ckandeler wrote:

> so what about changing this logic to get to enclosing decl instead (i.e. 
> TagDecl) and insert right after its end location?

That will break with nested classes, won't it?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.

kadircet wrote:

well, available for:
```
namespace {
struct Foo { void ba^r() {} };
}
```
and
```
namespace ns {
struct Foo { void ba^r() {} };
}
```
and
```
struct Foo { void ba^r() {} };
```

unavailable for:
```
namespace ns {
struct Foo { void bar(); };
void Foo::b^ar() {}
}
```

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
+  // Returns the most natural insertion point for \p QualifiedName in \p
+  // Contents. This currently cares about only the namespace proximity, but in
+  // feature it should also try to follow ordering of declarations. For 
example,
+  // if decls come in order `foo, bar, baz` then this function should return
+  // some point between foo and baz for inserting bar.
+  llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+   const Selection ) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
+if (SameFile) {
+  if (auto *Namespace = Source->getEnclosingNamespaceContext()) {

kadircet wrote:

yes, `getEnclosingNamespaceContext` can return `TranslationUnitDecl`, e.g. for:
```
struct Foo { void b^ar() {} };
```

and things are getting messy in that scenario, I missed the part around getting 
source locations, it'll actually be invalid in that case.

so what about changing this logic to get to enclosing decl instead (i.e. 
TagDecl) and insert right after its end location?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -349,6 +358,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace foo {
+namespace {
+struct Foo { void ba^r() {} };

kadircet wrote:

might be worth also inserting some extra declarations in between to see how 
insertion point selection works

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

https://github.com/ckandeler updated 
https://github.com/llvm/llvm-project/pull/69704

>From 27af98b5a9b71255b2873f25943ed23e42946b27 Mon Sep 17 00:00:00 2001
From: Christian Kandeler 
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
 files

Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
 .../clangd/refactor/tweaks/DefineOutline.cpp  | 135 ++
 .../unittests/tweaks/DefineOutlineTests.cpp   |  43 +-
 2 files changed, 119 insertions(+), 59 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..67d13f3eb4a6b07 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer , 
tok::TokenKind Kind,
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
   const syntax::TokenBuffer ,
   const HeuristicResolver *Resolver) {
   auto  = FD->getASTContext();
   auto  = AST.getSourceManager();
-  auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
-  if (!TargetContext)
-return error("define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 const NamedDecl *ND = Ref.Targets.front();
 const std::string Qualifier =
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()), ND);
 if (auto Err = DeclarationCleanups.add(
 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
   if (const auto *Destructor = llvm::dyn_cast(FD)) {
 if (auto Err = DeclarationCleanups.add(tooling::Replacement(
 SM, Destructor->getLocation(), 0,
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()),
  Destructor
   Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 
 struct InsertionPoint {
-  std::string EnclosingNamespace;
+  const DeclContext *EnclosingNamespace = nullptr;
   size_t Offset;
 };
-// Returns the most natural insertion point for \p QualifiedName in \p 
Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected getInsertionPoint(llvm::StringRef Contents,
- llvm::StringRef QualifiedName,
- const LangOptions ) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
-  assert(!Region.EligiblePoints.empty());
-  // FIXME: This selection can be made smarter by looking at the definition
-  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
-  // getEligibleRegions only knows about namespace begin/end events so we
-  // can't match function start/end positions yet.
-  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
-  if (!Offset)
-return Offset.takeError();
-  return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
 
 // Returns the range that should be deleted from declaration, which always
 // contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not a 

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.

ckandeler wrote:

Do you have suggestions? anon namespace in header is already covered.

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[Christian Kandeler via cfe-commits]

@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
+  // Returns the most natural insertion point for \p QualifiedName in \p
+  // Contents. This currently cares about only the namespace proximity, but in
+  // feature it should also try to follow ordering of declarations. For 
example,
+  // if decls come in order `foo, bar, baz` then this function should return
+  // some point between foo and baz for inserting bar.
+  llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+   const Selection ) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
+if (SameFile) {
+  if (auto *Namespace = Source->getEnclosingNamespaceContext()) {

ckandeler wrote:

You mean this also covers the global namespace? I wasn't sure about that. But 
don't we need special handling for that case, then? As I'd assume it won't have 
a proper location?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -349,6 +349,44 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Member function with some adornments
+  // FIXME: What's with the extra spaces?
+  {
+  "namespace {\n"

kadircet wrote:

can you use rawstring literals instead?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.

kadircet wrote:

can you add some availability tests here?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -349,6 +349,44 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Member function with some adornments
+  // FIXME: What's with the extra spaces?
+  {
+  "namespace {\n"

kadircet wrote:

no need for all the complicated test cases, let's just validate the insertion 
location here for a test case like:
foo.cc
```
namespace foo {
namespace {
struct Foo { void ba^r() {} };
}
}
```

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
+  // Returns the most natural insertion point for \p QualifiedName in \p
+  // Contents. This currently cares about only the namespace proximity, but in
+  // feature it should also try to follow ordering of declarations. For 
example,
+  // if decls come in order `foo, bar, baz` then this function should return
+  // some point between foo and baz for inserting bar.
+  llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+   const Selection ) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
+if (SameFile) {
+  if (auto *Namespace = Source->getEnclosingNamespaceContext()) {

kadircet wrote:

i think we should just fail the code action if enclosing namespace is nullptr, 
something is definitely wrong with that file (and falling back to textual 
matching feels suspicious).

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
+  // Returns the most natural insertion point for \p QualifiedName in \p
+  // Contents. This currently cares about only the namespace proximity, but in
+  // feature it should also try to follow ordering of declarations. For 
example,
+  // if decls come in order `foo, bar, baz` then this function should return
+  // some point between foo and baz for inserting bar.
+  llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+   const Selection ) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.

kadircet wrote:

can you move fixme to the function comment instead?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -435,14 +407,17 @@ class DefineOutline : public Tweak {
   if (MD->getParent()->isTemplated())
 return false;
 
-  // The refactoring is meaningless for unnamed classes and definitions
-  // within unnamed namespaces.
+  // The refactoring is meaningless for unnamed classes.

kadircet wrote:

`... and namespaces, unless we're outlining in the same file`

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

@@ -435,14 +407,17 @@ class DefineOutline : public Tweak {
   if (MD->getParent()->isTemplated())
 return false;
 
-  // The refactoring is meaningless for unnamed classes and definitions
-  // within unnamed namespaces.
+  // The refactoring is meaningless for unnamed classes.
   for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
 if (auto *ND = llvm::dyn_cast(DC)) {
-  if (ND->getDeclName().isEmpty())
+  if (ND->getDeclName().isEmpty() &&
+  (!SameFile || !llvm::dyn_cast(ND)))
 return false;
 }
   }
+} else if (SameFile) {

kadircet wrote:

nit: might be better to move this to the top, e.g:
```
auto *MD = dyn_cast...;
if (!MD)
  return !SameFile; // Can't outline free-standing functions in the same file.

// rest of the verification for methods
```

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

https://github.com/kadircet commented:

thanks, LG in general just some small adjustments

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

https://github.com/kadircet edited 
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

ckandeler wrote:

The new patch set inserts at the end of the namespace block for the same-file 
case.

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

https://github.com/ckandeler updated 
https://github.com/llvm/llvm-project/pull/69704

>From b88cdbcd106e27d3e594dc06824df10d77f9402b Mon Sep 17 00:00:00 2001
From: Christian Kandeler 
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
 files

Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
 .../clangd/refactor/tweaks/DefineOutline.cpp  | 115 +++---
 .../unittests/tweaks/DefineOutlineTests.cpp   |  40 +-
 2 files changed, 107 insertions(+), 48 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..7a64cf6cd006fd6 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer , 
tok::TokenKind Kind,
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
   const syntax::TokenBuffer ,
   const HeuristicResolver *Resolver) {
   auto  = FD->getASTContext();
   auto  = AST.getSourceManager();
-  auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
-  if (!TargetContext)
-return error("define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 const NamedDecl *ND = Ref.Targets.front();
 const std::string Qualifier =
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()), ND);
 if (auto Err = DeclarationCleanups.add(
 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
   if (const auto *Destructor = llvm::dyn_cast(FD)) {
 if (auto Err = DeclarationCleanups.add(tooling::Replacement(
 SM, Destructor->getLocation(), 0,
-getQualification(AST, *TargetContext,
+getQualification(AST, TargetContext,
  SM.getLocForStartOfFile(SM.getMainFileID()),
  Destructor
   Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, 
llvm::StringRef TargetNamespace,
 }
 
 struct InsertionPoint {
-  std::string EnclosingNamespace;
+  const DeclContext *EnclosingNamespace = nullptr;
   size_t Offset;
 };
-// Returns the most natural insertion point for \p QualifiedName in \p 
Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected getInsertionPoint(llvm::StringRef Contents,
- llvm::StringRef QualifiedName,
- const LangOptions ) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
-  assert(!Region.EligiblePoints.empty());
-  // FIXME: This selection can be made smarter by looking at the definition
-  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
-  // getEligibleRegions only knows about namespace begin/end events so we
-  // can't match function start/end positions yet.
-  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
-  if (!Offset)
-return Offset.takeError();
-  return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
 
 // Returns the range that should be deleted from declaration, which always
 // contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not a 

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

kadircet wrote:

Yes, the current logic that finds insertion point wont work in presence of anon 
namespaces.

We normally do some pseudo parsing to find a location to insert the definiton, 
but because these are happening inside the main-file now, you can directly make 
use of sourcelocations inside the AST to determine an insertion point.

The other part is spelling of the function name and return type, there's a good 
chance they'll also do a weird thing when used for entities inside anon 
namespaces.

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

ckandeler wrote:

> Would you mind evolving the patch (and the infra) to work for 
> method-definitions inside anon namespaces?

Of course not; I didn't even now it wasn't working. 
I suppose the main point is to determine the insertion location such that it is 
still within the original namespace?
(I haven't looked at that part of the code so far.)

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[kadir çetinkaya via cfe-commits]

kadircet wrote:

Thanks! I believe the idea is great, but I am not really sure if this is useful 
enough without handling anon namespaces properly. It's very rare that someone 
has a class/struct in a cc file, that isn't in an anon-namespace.

Would you mind evolving the patch (and the infra) to work for 
method-definitions inside anon namespaces?

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

ckandeler wrote:

ping

https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clangd

Author: None (ckandeler)


Changes

Moving the body of member functions out-of-line makes sense for classes defined 
in implementation files too.

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


2 Files Affected:

- (modified) clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
(+18-15) 
- (modified) clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp 
(+31-1) 


``diff
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..a52bd5ee46f0ce2 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -409,14 +409,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not a in-line function definition.
 if (!Source || !Source->doesThisDeclarationHaveABody() ||
 Source->isOutOfLine())
@@ -443,6 +438,9 @@ class DefineOutline : public Tweak {
 return false;
 }
   }
+} else if (SameFile) {
+  // Bail out for free-standing functions in non-header file.
+  return false;
 }
 
 // Note that we don't check whether an implementation file exists or not in
@@ -453,8 +451,8 @@ class DefineOutline : public Tweak {
 
   Expected apply(const Selection ) override {
 const SourceManager  = Sel.AST->getSourceManager();
-auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
-
+auto CCFile = SameFile ? Sel.AST->tuPath().str()
+   : getSourceFile(Sel.AST->tuPath(), Sel);
 if (!CCFile)
   return error("Couldn't find a suitable implementation file.");
 assert(Sel.FS && "FS Must be set in apply");
@@ -499,17 +497,22 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
 private:
   const FunctionDecl *Source = nullptr;
+  bool SameFile = false;
 };
 
 REGISTER_TWEAK(DefineOutline)
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp 
b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index d1e60b070f20e95..8aaadb66943e1ee 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.
   EXPECT_UNAVAILABLE(R"cpp(
 [[void [[f^o^o]]() [[{
   return;
@@ -349,6 +349,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Member function with some adornments
+  // FIXME: What's with the extra spaces?
+  {"#define OVERRIDE override\n"
+   "struct Base { virtual int func() const = 0; };\n"
+   "struct Derived : Base {\n"
+   "   inline int f^unc() const OVERRIDE { return 42; }\n"
+   "};\n"
+   "int main() {}\n",
+   "#define OVERRIDE override\n"
+   "struct Base { virtual int func() const = 0; };\n"
+   "struct Derived : Base {\n"
+   "int func() const OVERRIDE ;\n"
+   "};\n"
+   "int main() {}\n"
+   " int Derived::func() const  { return 42; }\n"},
+  };
+
+  for (const auto  : Cases) {
+SCOPED_TRACE(Case.Test);
+EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource);
+  }
+}
+
 TEST_F(DefineOutlineTest, HandleMacros) {
   llvm::StringMap EditedFiles;
   

[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)

[via cfe-commits]

https://github.com/ckandeler created 
https://github.com/llvm/llvm-project/pull/69704

Moving the body of member functions out-of-line makes sense for classes defined 
in implementation files too.

>From 0b7df6c0a713c1c40f6c92d958fa6a96796ed80f Mon Sep 17 00:00:00 2001
From: Christian Kandeler 
Date: Thu, 19 Oct 2023 17:51:11 +0200
Subject: [PATCH] [clangd] Allow "move function body out-of-line" in non-header
 files

Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
---
 .../clangd/refactor/tweaks/DefineOutline.cpp  | 33 ++-
 .../unittests/tweaks/DefineOutlineTests.cpp   | 32 +-
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..a52bd5ee46f0ce2 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -409,14 +409,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection ) override {
-// Bail out if we are not in a header file.
-// FIXME: We might want to consider moving method definitions below class
-// definition even if we are inside a source file.
-if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-  Sel.AST->getLangOpts()))
-  return false;
-
+SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
 Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
 // Bail out if the selection is not a in-line function definition.
 if (!Source || !Source->doesThisDeclarationHaveABody() ||
 Source->isOutOfLine())
@@ -443,6 +438,9 @@ class DefineOutline : public Tweak {
 return false;
 }
   }
+} else if (SameFile) {
+  // Bail out for free-standing functions in non-header file.
+  return false;
 }
 
 // Note that we don't check whether an implementation file exists or not in
@@ -453,8 +451,8 @@ class DefineOutline : public Tweak {
 
   Expected apply(const Selection ) override {
 const SourceManager  = Sel.AST->getSourceManager();
-auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
-
+auto CCFile = SameFile ? Sel.AST->tuPath().str()
+   : getSourceFile(Sel.AST->tuPath(), Sel);
 if (!CCFile)
   return error("Couldn't find a suitable implementation file.");
 assert(Sel.FS && "FS Must be set in apply");
@@ -499,17 +497,22 @@ class DefineOutline : public Tweak {
   HeaderUpdates = HeaderUpdates.merge(*DelInline);
 }
 
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
-  return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
-   std::move(HeaderFE->second));
+if (SameFile) {
+  tooling::Replacements  = Effect->ApplyEdits[*CCFile].Replacements;
+  R = R.merge(HeaderUpdates);
+} else {
+  auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+  if (!HeaderFE)
+return HeaderFE.takeError();
+  Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
 return std::move(*Effect);
   }
 
 private:
   const FunctionDecl *Source = nullptr;
+  bool SameFile = false;
 };
 
 REGISTER_TWEAK(DefineOutline)
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp 
b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index d1e60b070f20e95..8aaadb66943e1ee 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.
   EXPECT_UNAVAILABLE(R"cpp(
 [[void [[f^o^o]]() [[{
   return;
@@ -349,6 +349,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Member function with some adornments
+  // FIXME: What's with the extra spaces?
+  {"#define OVERRIDE override\n"
+   "struct Base { virtual int func() const = 0; };\n"
+   "struct Derived : Base {\n"
+   "   inline int f^unc() const OVERRIDE { return 42; }\n"
+   "};\n"
+   "int main() {}\n",
+   "#define OVERRIDE override\n"
+   "struct Base { virtual int func() const = 0; };\n"
+   "struct Derived : Base {\n"
+   "int func() const OVERRIDE ;\n"
+   "};\n"
+   "int main() {}\n"
+   " int Derived::func() const  {