[clangd] Support non-trivial defaulted special member functions in Define Outline tweak

[njames93]

Summary

Sometimes classes can't have a defaulted or empty special member function declared inside the class if any of the members have forward declared types as template parameters.
The Pimpl idiom is a classic example of this
In these situations it makes sense to have a defaulted special member function moved to the source file. Currently the define outline tweak will reject these defaulted special member functions.

[kadircet]

there's actually a slight difference between an inline defaulted special member function and an out-of-line defaulted one. the latter makes the special member "user-defined" which might cause various headaches (e.g. type is no longer "trivial"). i don't think we should make it easy for people to change trivialness of types (especially considering our experience with people thinking anything that comes from the lightbulb is a way to improve your code). on a side node, this is less of an issue in the scenario you described (i.e. when the destructor is virtual, as it already means type is non-trivial).

so if you feel like this is really useful, i guess we can change the scope a little to apply to all special members, but only when type is no longer trivial. WDYT?

[njames93]

In D147808#4268523, @kadircet wrote:

there's actually a slight difference between an inline defaulted special member function and an out-of-line defaulted one. the latter makes the special member "user-defined" which might cause various headaches (e.g. type is no longer "trivial"). i don't think we should make it easy for people to change trivialness of types (especially considering our experience with people thinking anything that comes from the lightbulb is a way to improve your code). on a side node, this is less of an issue in the scenario you described (i.e. when the destructor is virtual, as it already means type is non-trivial).

so if you feel like this is really useful, i guess we can change the scope a little to apply to all special members, but only when type is no longer trivial. WDYT?

That makes sense.

[njames93]

Updated to work on all special member functions as long as they aren't trivial

[njames93]

Fix unittest, tests had trivial and non trivial mixed up

[njames93]

Fix clang-format issues. Ping?

[kadircet]

thanks a lot, and sorry for the delay!

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

328

can you add a comment here saying // We should have just `= default` in this case

404

i know i asked for triviality check in the first version, but thinking a little bit more about it, users might be more confused about code action's availability. as this is very subtle at a glance for anyone but the compiler. so i think it's better to just emit it always.

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

llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
   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
 // initializers.
 SourceRange getDeletionRange(const FunctionDecl *FD,
                              const syntax::TokenBuffer &TokBuf) {
+  if (FD->isExplicitlyDefaulted()) {
+    auto Toks =
+        TokBuf.expandedTokens({FD->getTypeSpecEndLoc(), FD->getDefaultLoc()});
+    Toks = Toks.drop_until(
+        [](const syntax::Token &Tok) { return Tok.kind() == tok::equal; });
+    assert(Toks.size() == 2);
+    return {Toks.front().location(), Toks.back().endLocation()};
+  }
   auto DeletionRange = FD->getBody()->getSourceRange();
   if (auto *CD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
     // AST doesn't contain the location for ":" in ctor initializers. Therefore
     // we find it by finding the first ":" before the first ctor initializer.
     SourceLocation InitStart;
     // Find the first initializer.
     for (const auto *CInit : CD->inits()) {
       // SourceOrder is -1 for implicit initializers.
bool prepare(const Selection &Sel) override {
     // 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;
 
     Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
     // Bail out if the selection is not a in-line function definition.
-    if (!Source || !Source->doesThisDeclarationHaveABody() ||
+    // Defaulted destrucors are OK as they sometimes need to be defined out of
+    // line.
+    if (!Source ||
+        (!Source->doesThisDeclarationHaveABody() &&
+         (!isa<CXXDestructorDecl>(Source) ||
+          !Source->isExplicitlyDefaulted())) ||
         Source->isOutOfLine())
       return false;
 
     // Bail out if this is a function template or specialization, as their
     // definitions need to be visible in all including translation units.
     if (Source->getDescribedFunctionTemplate())
       return false;
     if (Source->getTemplateSpecializationInfo())

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

llvm::StringRef ExpectedSource;
           "  Foo::Foo(int) {}\n",
       },
       // Destrctors
       {
           "class A { ~A^(){} };",
           "class A { ~A(); };",
           "A::~A(){} ",
       },
+      {
+          "class A { ~A^() = default; };",
+          "class A { ~A() ; };",
+          "A::~A() = default;",
+      },
   };
   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)));
   }
 }