[clangd] Allow "move function body out-of-line" in non-header files

[ckandeler]

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

[llvmbot]

@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 --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"] = "";

[ckandeler]

ping

[kadircet]

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?

[ckandeler]

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.)

[kadircet]

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.

[ckandeler]

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

[kadircet]

thanks, LG in general just some small adjustments

[ckandeler]

ping

[ckandeler]

ping

[ckandeler]

ping

[ckandeler]

ping

[ckandeler]

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.