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

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clangd

Author: None (ckandeler)

<details>
<summary>Changes</summary>

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 &Sel) 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<Effect> apply(const Selection &Sel) override {
     const SourceManager &SM = 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 &R = 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 &Case : Cases) {
+    SCOPED_TRACE(Case.Test);
+    EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource);
+  }
+}
+
 TEST_F(DefineOutlineTest, HandleMacros) {
   llvm::StringMap<std::string> EditedFiles;
   ExtraFiles["Test.cpp"] = "";

``````````

</details>


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