[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGddcce0f3d665: [clangd] Define out-of-line qualify function 
name (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D70656?vs=231513&id=232042#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2004,7 +2004,7 @@
 Bar foo() ;
   };
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -2022,6 +2022,58 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef TestHeader;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+  "",
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+  "void a::b::Foo::foo() {}\n",
+  },
+  {
+  "namespace a { namespace b { void f^oo() {} } }",
+  "namespace a{}",
+  "namespace a { namespace b { void foo() ; } }",
+  "namespace a{void b::foo() {} }",
+  },
+  {
+  "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() {} ",
+  },
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+ExtraFiles["Test.cpp"] = Case.TestSource;
+EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)))
+<< Case.TestHeader;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -136,7 +136,6 @@
 // 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.
-// FIXME: Qualify function name depending on the target context.
 llvm::Expected
 getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
@@ -149,16 +148,17 @@
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements QualifierInsertions;
 
-  // Finds the first unqualified name in function return type and qualifies it
-  // to be valid in TargetContext.
+  // Finds the first unqualified name in function return type and name, then
+  // qualifies those to be valid in TargetContext.
   findExplicitReferences(FD, [&](ReferenceLoc Ref) {
 // It is enough to qualify the first qualifier, so skip references with a
 // qualifier. Also we can't do much if there are no targets or name is
 // inside a macro body.
 if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
   return;
-// Qualify return type
-if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+// Only qualify return type and function name.
+if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+Ref.NameLoc != FD->getLocation())
   return;
 
 for (const NamedDecl *ND : Ref.Targets) {
@@ -293,9 +293,7 @@
 auto FuncDef =
 getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
 if (!FuncDef)
-  return llvm::createStringError(
-  llvm::inconvertibleErrorCode(),
-  "Couldn't get full source for function definition.");
+  return FuncDef.takeError();
 
 SourceManagerForFile SMFF(*CCFile, Contents);
 const tooling::Replacement InsertFunctionDef(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.ll

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 231513.
kadircet added a comment.

- Update comments after rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2006,7 +2006,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -2024,6 +2024,58 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef TestHeader;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+  "",
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+  "void a::b::Foo::foo() {}\n",
+  },
+  {
+  "namespace a { namespace b { void f^oo() {} } }",
+  "namespace a{}",
+  "namespace a { namespace b { void foo() ; } }",
+  "namespace a{void b::foo() {} }",
+  },
+  {
+  "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() {} ",
+  },
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+ExtraFiles["Test.cpp"] = Case.TestSource;
+EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)))
+<< Case.TestHeader;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -107,7 +107,6 @@
 // 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.
-// FIXME: Qualify function name depending on the target context.
 llvm::Expected
 getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
@@ -120,16 +119,17 @@
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements QualifierInsertions;
 
-  // Finds the first unqualified name in function return type and qualifies it
-  // to be valid in TargetContext.
+  // Finds the first unqualified name in function return type and name, then
+  // qualifies those to be valid in TargetContext.
   findExplicitReferences(FD, [&](ReferenceLoc Ref) {
 // It is enough to qualify the first qualifier, so skip references with a
 // qualifier. Also we can't do much if there are no targets or name is
 // inside a macro body.
 if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
   return;
-// Qualify return type
-if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+// Only qualify return type and function name.
+if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+Ref.NameLoc != FD->getLocation())
   return;
 
 for (const NamedDecl *ND : Ref.Targets) {
@@ -284,9 +284,7 @@
 auto FuncDef =
 getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
 if (!FuncDef)
-  return llvm::createStringError(
-  llvm::inconvertibleErrorCode(),
-  "Couldn't get full source for function definition.");
+  return FuncDef.takeError();
 
 SourceManagerForFile SMFF(*CCFile, Contents);
 const tooling::Replacement InsertFunctionDef(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 231512.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2006,7 +2006,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -2024,6 +2024,58 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef TestHeader;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+  "",
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+  "void a::b::Foo::foo() {}\n",
+  },
+  {
+  "namespace a { namespace b { void f^oo() {} } }",
+  "namespace a{}",
+  "namespace a { namespace b { void foo() ; } }",
+  "namespace a{void b::foo() {} }",
+  },
+  {
+  "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() {} ",
+  },
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+ExtraFiles["Test.cpp"] = Case.TestSource;
+EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)))
+<< Case.TestHeader;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -128,8 +128,9 @@
 // inside a macro body.
 if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
   return;
-// Qualify return type
-if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+// Only qualify return type and function name.
+if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+Ref.NameLoc != FD->getLocation())
   return;
 
 for (const NamedDecl *ND : Ref.Targets) {
@@ -284,9 +285,7 @@
 auto FuncDef =
 getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
 if (!FuncDef)
-  return llvm::createStringError(
-  llvm::inconvertibleErrorCode(),
-  "Couldn't get full source for function definition.");
+  return FuncDef.takeError();
 
 SourceManagerForFile SMFF(*CCFile, Contents);
 const tooling::Replacement InsertFunctionDef(


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2006,7 +2006,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -2024,6 +2024,58 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef TestHeader;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+  "",
+  R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+  "void a::b::Foo::foo() {}\n",
+  },
+  {
+  "namespace a { n

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1831
+  struct {
+llvm::StringRef Test;
+llvm::StringRef SourceFile;

maybe name them `TestHeader`, and `TestSource`.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1854
+   "void a::b::Foo::foo() {}\n"},
+  {"namespace a { namespace b { void f^oo() {} } }", "namespace a{}",
+   "namespace a { namespace b { void foo() ; } }",

nit: could we keep each string per line? It would improve the code readability 
here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - Could not check out parent git hash 
"0a2c8c14d3aa16ca964a8e643f9a5bfdd5459de3". 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 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: FAILURE - Could not check out parent git hash 
"0a2c8c14d3aa16ca964a8e643f9a5bfdd5459de3". 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 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.
kadircet updated this revision to Diff 230843.
kadircet added a comment.

- Get rid of debug output


When moving function definitions to a different context, the function
name might need a different spelling, for example in the header it might be:

  namespace a {
void foo() {}
  }

And we might want to move it into a context which doesn't have `namespace a` as
a parent, then we must re-spell the function name, i.e:

  void a::foo() {}

This patch implements a version of this which ignores using namespace
declarations in the source file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70656

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1807,7 +1807,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -1825,6 +1825,50 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef Test;
+llvm::StringRef SourceFile;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+   "",
+   R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+   "void a::b::Foo::foo() {}\n"},
+  {"namespace a { namespace b { void f^oo() {} } }", "namespace a{}",
+   "namespace a { namespace b { void foo() ; } }",
+   "namespace a{void b::foo() {} }"},
+  {"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() {} "},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+ExtraFiles["Test.cpp"] = Case.SourceFile;
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)))
+<< Case.Test;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -132,8 +132,9 @@
 // inside a macro body.
 if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
   return;
-// Qualify return type
-if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+// Only qualify return type and function name.
+if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+Ref.NameLoc != FD->getLocation())
   return;
 
 for (const NamedDecl *ND : Ref.Targets) {
@@ -178,8 +179,6 @@
   if (!QualifiedFunc)
 return QualifiedFunc.takeError();
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
-
-  // FIXME: Qualify function name depending on the target context.
 }
 
 struct InsertionPoint {
@@ -285,9 +284,7 @@
 auto FuncDef =
 getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
 if (!FuncDef)
-  return llvm::createStringError(
-  llvm::inconvertibleErrorCode(),
-  "Couldn't get full source for function definition.");
+  return FuncDef.takeError();
 
 SourceManagerForFile SMFF(*CCFile, Contents);
 const tooling::Replacement InsertFunctionDef(


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1807,7 +1807,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -1825,6 +1825,50 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm

[PATCH] D70656: [clangd] Define out-of-line qualify function name

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 230843.
kadircet added a comment.

- Get rid of debug output


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70656/new/

https://reviews.llvm.org/D70656

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1807,7 +1807,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -1825,6 +1825,50 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef Test;
+llvm::StringRef SourceFile;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+   "",
+   R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+   "void a::b::Foo::foo() {}\n"},
+  {"namespace a { namespace b { void f^oo() {} } }", "namespace a{}",
+   "namespace a { namespace b { void foo() ; } }",
+   "namespace a{void b::foo() {} }"},
+  {"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() {} "},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+ExtraFiles["Test.cpp"] = Case.SourceFile;
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)))
+<< Case.Test;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -132,8 +132,9 @@
 // inside a macro body.
 if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
   return;
-// Qualify return type
-if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+// Only qualify return type and function name.
+if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+Ref.NameLoc != FD->getLocation())
   return;
 
 for (const NamedDecl *ND : Ref.Targets) {
@@ -178,8 +179,6 @@
   if (!QualifiedFunc)
 return QualifiedFunc.takeError();
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
-
-  // FIXME: Qualify function name depending on the target context.
 }
 
 struct InsertionPoint {
@@ -285,9 +284,7 @@
 auto FuncDef =
 getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
 if (!FuncDef)
-  return llvm::createStringError(
-  llvm::inconvertibleErrorCode(),
-  "Couldn't get full source for function definition.");
+  return FuncDef.takeError();
 
 SourceManagerForFile SMFF(*CCFile, Contents);
 const tooling::Replacement InsertFunctionDef(


Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1807,7 +1807,7 @@
 Bar foo() ;
   }
 })cpp",
-   "a::Foo::Bar foo() { return {}; }\n"},
+   "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
   {R"cpp(
 class Foo;
 Foo fo^o() { return; })cpp",
@@ -1825,6 +1825,50 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+llvm::StringRef Test;
+llvm::StringRef SourceFile;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void fo^o() {}
+};
+  }
+})cpp",
+   "",
+   R"cpp(
+namespace a {
+  namespace b {
+class Foo {
+  void foo() ;
+};
+  }
+})cpp",
+   "void a::b::Foo::foo() {}\n"},
+  {"namespace a { namespace b { void f^oo() {} } }"