[clangd] Delete ctor initializers while moving functions out-of-line

[kadircet]

Summary

Currently we only delete function body from declaration, in addition to
that we should also drop ctor initializers.

Unfortunately CXXConstructorDecl doesn't store the location of : before
initializers, therefore we make use of token buffer to figure out where to start
deletion.

https://github.com/clangd/clangd/issues/220

[merge_guards_bot]

Build result: FAILURE - Could not check out parent git hash "8d52e0ffa278d9047731523cc72ce605f8f336df". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

[hokein]

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

277

There is an alternative here, I think. The end loc of FunctionProtoTypeLoc should point the location after the ), e.g. the Foo()^ : , would it be easier to use it as as the InitStart?

it can be retrieved by CD->getTypeSourceInfo()->getTypeLoc().getEndLoc().

[kadircet]

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

277

yes, that would be great if attributes didn't exist. e.g.:

cpp
class Foo {
  Foo() __attribute__((weak)) : z(2) {}
  int z;
};

for the example given above, typeloc.endloc would not include trailing attributes, updated test case according to that.(And added a fixme, since we should not propagate those attributes into definition)

also the approach proposed in this patch would result in minimal changes, by keeping anything before : in declaration site.
if we were to make use of endloc we could end up deleting more tokens in between, even if it would be rare to have tokens in between.

[kadircet]

• Address comments

[merge_guards_bot]

Build result: fail - 60625 tests passed, 1 failed and 726 were skipped.

failed: LLVM.CodeGen/RISCV/mir-target-flags.ll

Log files: console-log.txt, CMakeCache.txt

[hokein]

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

271

nit: the same, llvm::find_if.

277

ah, I see, that makes sense.

[kadircet]

• Address comments

[merge_guards_bot]

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

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

 #include "ParsedAST.h"
 #include "Path.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
getFunctionSourceAfterReplacements(const FunctionDecl *FD,
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
 // Creates a modified version of function definition that can be inserted at a
 // different location, qualifies return value and function name to achieve that.
 // Contains function signature, except defaulted parameter arguments, body and
 // template parameters if applicable. No need to qualify parameters, as they are
 // looked up in the context containing the function/method.
+// FIXME: Drop attributes in function signature.
 llvm::Expected<std::string>
 getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
                       const syntax::TokenBuffer &TokBuf) {
   auto &AST = FD->getASTContext();
   auto &SM = AST.getSourceManager();
   auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
   if (!TargetContext)
     return llvm::createStringError(
getInsertionPoint(llvm::StringRef Contents, llvm::StringRef QualifiedName,
   // 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
+// initializers.
+SourceRange getDeletionRange(const FunctionDecl *FD,
+                             const syntax::TokenBuffer &TokBuf) {
+  auto DeletionRange = FD->getBody()->getSourceRange();
+  if (auto *CD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
+    const auto &SM = TokBuf.sourceManager();
+    // 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()) {
+      // We don't care about in-class initializers.
+      if (CInit->isInClassMemberInitializer())
+        continue;
+      if (InitStart.isInvalid() ||
+          SM.isBeforeInTranslationUnit(CInit->getSourceLocation(), InitStart))
+        InitStart = CInit->getSourceLocation();
+    }
+    if (InitStart.isValid()) {
+      auto Toks = TokBuf.expandedTokens(CD->getSourceRange());
+      // Drop any tokens after the initializer.
+      Toks = Toks.take_while([&TokBuf, &InitStart](const syntax::Token &Tok) {
+        return TokBuf.sourceManager().isBeforeInTranslationUnit(Tok.location(),
+                                                                InitStart);
+      });
+      // Look for the first colon.
+      auto Tok =
+          llvm::find_if(llvm::reverse(Toks), [](const syntax::Token &Tok) {
+            return Tok.kind() == tok::colon;
+          });
+      assert(Tok != Toks.rend());
+      DeletionRange.setBegin(Tok->location());
+    }
+  }
+  return DeletionRange;
+}
+
 /// Moves definition of a function/method to an appropriate implementation file.
 ///
 /// Before:
 /// a.h
 ///   void foo() { return; }
 /// a.cc
 ///   #include "a.h"
 ///
auto Effect = Effect::mainFileEdit(
         SMFF.get(), tooling::Replacements(InsertFunctionDef));
     if (!Effect)
       return Effect.takeError();
 
     // FIXME: We should also get rid of inline qualifier.
     const tooling::Replacement DeleteFuncBody(
         Sel.AST.getSourceManager(),
         CharSourceRange::getTokenRange(*toHalfOpenFileRange(
-            SM, Sel.AST.getLangOpts(), Source->getBody()->getSourceRange())),
+            SM, Sel.AST.getLangOpts(),
+            getDeletionRange(Source, Sel.AST.getTokens()))),
         ";");
     auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(),
                                      tooling::Replacements(DeleteFuncBody));
     if (!HeaderFE)
       return HeaderFE.takeError();
 
     Effect->ApplyEdits.try_emplace(HeaderFE->first,
                                    std::move(HeaderFE->second));

clang-tools-extra/clangd/unittests/TweakTests.cpp

llvm::StringRef ExpectedSource;
           "template <> void foo<int>() { return; }",
       },
       // Default args.
       {
           "void fo^o(int x, int y = 5, int = 2, int (*foo)(int) = nullptr) {}",
           "void foo(int x, int y = 5, int = 2, int (*foo)(int) = nullptr) ;",
           "void foo(int x, int y , int , int (*foo)(int) ) {}",
       },
+      // Ctor initializers.
+      {
+          R"cpp(
+              class Foo {
+                int y = 2;
+                F^oo(int z) __attribute__((weak)) : bar(2){}
+                int bar;
+                int z = 2;
+              };)cpp",
+          R"cpp(
+              class Foo {
+                int y = 2;
+                Foo(int z) __attribute__((weak)) ;
+                int bar;
+                int z = 2;
+              };)cpp",
+          "Foo::Foo(int z) __attribute__((weak)) : bar(2){}\n",
+      },
   };
   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)));
   }
 }