[clangd] Handle destructors in DefineOutline tweak

[njames93]

Summary

Fix destructors being incorrectly defined in the DefineOutline tweak
Currently it doesn't prepend the class name to the destructor

class A { ~A() {} };
// Destructor definition after outline
~A() {}
// After this fix
A::~A() {}

[njames93]

Clang-format

[njames93]

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

186–194

I had thought about adjusting the FindTarget code for this, but that resulted in a lot of other failed tests. It seems a delicate one there.

[kadircet]

thanks!

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

186–194

yeah it's a little unfortunate. can you have a comment about it in here so that we can keep it in mind. something like:

findExplicitReferences doesn't provide references to constructor/destructors, it only provides references to type names inside them.
this works for constructors, but doesn't work for destructor as type name doesn't cover leading `~`, so handle it specially.

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

findExplicitReferences(
             getQualification(AST, *TargetContext,
                              SM.getLocForStartOfFile(SM.getMainFileID()), ND);
         if (auto Err = DeclarationCleanups.add(
                 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
           Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
       },
       Resolver);
 
+  // findExplicitReferences doesn't provide references to
+  // constructor/destructors, it only provides references to type names inside
+  // them.
+  // this works for constructors, but doesn't work for destructor as type name
+  // doesn't cover leading `~`, so handle it specially.
+  if (const auto *Destructor = llvm::dyn_cast<CXXDestructorDecl>(FD)) {
+    if (auto Err = DeclarationCleanups.add(tooling::Replacement(
+            SM, Destructor->getLocation(), 0,
+            getQualification(AST, *TargetContext,
+                             SM.getLocForStartOfFile(SM.getMainFileID()),
+                             Destructor))))
+      Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+  }
+
   // Get rid of default arguments, since they should not be specified in
   // out-of-line definition.
   for (const auto *PVD : FD->parameters()) {
     if (!PVD->hasDefaultArg())
       continue;
     // Deletion range spans the initializer, usually excluding the `=`.
     auto DelRange = CharSourceRange::getTokenRange(PVD->getDefaultArgRange());
     // Get all tokens before the default argument.

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

llvm::StringRef ExpectedSource;
               explicit explicit Fo^o(int) {}
             };)cpp",
           R"cpp(
             struct Foo {
               explicit explicit Foo(int) ;
             };)cpp",
           "  Foo::Foo(int) {}\n",
       },
+      // Destrctors
+      {
+          "class A { ~A^(){} };",
+          "class A { ~A(); };",
+          "A::~A(){} ",
+      },
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
llvm::StringRef ExpectedSource;
       {
           "namespace a { namespace b { void f^oo() {} } }",
           "using namespace a;",
           "namespace a { namespace b { void foo() ; } }",
           // FIXME: Take using namespace directives in the source file into
           // account. This can be spelled as b::foo instead.
           "using namespace a;void a::b::foo() {} ",
       },
+      {
+          "namespace a { class A { ~A^(){} }; }",
+          "",
+          "namespace a { class A { ~A(); }; }",
+          "a::A::~A(){} ",
+      },
+      {
+          "namespace a { class A { ~A^(){} }; }",
+          "namespace a{}",
+          "namespace a { class A { ~A(); }; }",
+          "namespace a{A::~A(){} }",
+      },
   };
   llvm::StringMap<std::string> EditedFiles;
   for (auto &Case : Cases) {
     ExtraFiles["Test.cpp"] = std::string(Case.TestSource);
     EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)))
         << Case.TestHeader;