[clangd] Delete default arguments while moving functions out-of-line

[kadircet]

Summary

Only function declarations should have the default arguments.

This patch makes sure we don't propogate those arguments to out-of-line
definitions.

Fixes https://github.com/clangd/clangd/issues/221

[merge_guards_bot]

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

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

[ilya-biryukov]

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

1938

NIT: missing full period at the end of the comment

[kadircet]

• Add . after comment and fix indentation.

[merge_guards_bot]

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

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

[hokein]

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

190

just curious what's the getDefaultArgRange for case like void foo(int a = 5), is it just [[5]]?

195

The approach seems fail on the following case, StartLoc will point to Foo.

class Bar { void func(int); };
void bind(const char*, void(Bar::*Foo)(int) = nullptr)

[kadircet]

• Make use of token buffer instead of trying to go-over identifier name.

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

190

yes it is the range of the initializer.

195

ah, right. switching to a similar strategy as D71188 to find first = before initializer.

[merge_guards_bot]

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

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

[hokein]

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

199

nit: llvm::find_if(llvm::reverse(Tokens), [](Tok) { return Toke.kind() == tok::equal; })

[kadircet]

• Address comments

[merge_guards_bot]

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

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

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

 //===--- DefineOutline.cpp ---------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 #include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
 #include "Logger.h"
+#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/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"
 #include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include <cstddef>
 #include <string>
 
 namespace clang {
 namespace clangd {
auto QualifiedFunc = tooling::applyAllReplacements(
       SM.getBufferData(SM.getMainFileID()), Replacements);
   if (!QualifiedFunc)
     return QualifiedFunc.takeError();
   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, body and template parameters if applicable.
-// No need to qualify parameters, as they are looked up in the context
-// containing the function/method.
+// 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.
 llvm::Expected<std::string>
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
-  auto &SM = FD->getASTContext().getSourceManager();
+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(
         llvm::inconvertibleErrorCode(),
         "define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements QualifierInsertions;
findExplicitReferences(FD, [&](ReferenceLoc Ref) {
     for (const NamedDecl *ND : Ref.Targets) {
       if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
         elog("Targets from multiple contexts: {0}, {1}",
              printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND));
         return;
       }
     }
     const NamedDecl *ND = Ref.Targets.front();
-    const std::string Qualifier =
-        getQualification(FD->getASTContext(), *TargetContext,
-                         SM.getLocForStartOfFile(SM.getMainFileID()), ND);
+    const std::string Qualifier = getQualification(
+        AST, *TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), ND);
     if (auto Err = QualifierInsertions.add(
             tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
       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()) {
+      // Deletion range initially spans the initializer, excluding the `=`.
+      auto DelRange = CharSourceRange::getTokenRange(PVD->getDefaultArgRange());
+      // Get all tokens before the default argument.
+      auto Tokens = TokBuf.expandedTokens(PVD->getSourceRange())
+                        .take_while([&SM, &DelRange](const syntax::Token &Tok) {
+                          return SM.isBeforeInTranslationUnit(
+                              Tok.location(), DelRange.getBegin());
+                        });
+      // Find the last `=` before the default arg.
+      auto Tok =
+          llvm::find_if(llvm::reverse(Tokens), [](const syntax::Token &Tok) {
+            return Tok.kind() == tok::equal;
+          });
+      assert(Tok != Tokens.rend());
+      DelRange.setBegin(Tok->location());
+      if (auto Err =
+              QualifierInsertions.add(tooling::Replacement(SM, DelRange, "")))
+        Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+    }
+  }
+
   if (Errors)
     return std::move(Errors);
   return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
 }
 
 struct InsertionPoint {
   std::string EnclosingNamespace;
   size_t Offset;
if (!Buffer)
                                      Buffer.getError().message());
     auto Contents = Buffer->get()->getBuffer();
     auto InsertionPoint =
         getInsertionPoint(Contents, Source->getQualifiedNameAsString(),
                           getFormatStyleForFile(*CCFile, Contents, &FS));
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
-    auto FuncDef =
-        getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
+    auto FuncDef = getFunctionSourceCode(
+        Source, InsertionPoint->EnclosingNamespace, Sel.AST.getTokens());
     if (!FuncDef)
       return FuncDef.takeError();
 
     SourceManagerForFile SMFF(*CCFile, Contents);
     const tooling::Replacement InsertFunctionDef(
         *CCFile, InsertionPoint->Offset, 0, *FuncDef);
     auto Effect = Effect::mainFileEdit(
         SMFF.get(), tooling::Replacements(InsertFunctionDef));

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

llvm::StringRef ExpectedSource;
           R"cpp(
             template <typename> void foo();
             template <> void fo^o<int>() { return; })cpp",
           R"cpp(
             template <typename> void foo();
             template <> void foo<int>() ;)cpp",
           "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) ) {}",
+      },
   };
   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)));
   }
 }