[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1972,6 +1972,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  };
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  };
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -57,31 +64,136 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
-// Creates a modified version of function definition that can be inserted at a
-// different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
-  auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-   FD->getSourceRange());
-  if (!CharRange)
+// Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
+// for global namespace, and endwith "::" otherwise.
+// Returns None if TargetNS is not a prefix of CurContext.
+llvm::Optional
+findContextForNS(llvm::StringRef TargetNS, const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  // If TargetNS is empty, it means global ns, which is translation unit.
+  if (TargetNS.empty()) {
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+return CurContext;
+  }
+  // Otherwise we need to drop any trailing namespaces from CurContext until
+  // we reach TargetNS.
+  std::string TargetContextNS =
+  CurContext->isNamespace()
+  ? llvm::cast(CurContext)->getQualifiedNameAsString()
+  : "";
+  TargetContextNS.append("::");
+
+  llvm::StringRef CurrentContextNS(TargetContextNS);
+  // If TargetNS is not a prefix of CurrentContext, there's no way to reach
+  // it.
+  if (!CurrentContextNS.startswith(TargetNS))
 return llvm::None;
+
+  while (CurrentContextNS != Targe

[PATCH] D70535: [clangd] Define out-of-line qualify return value

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

merge_guards_bot added a comment.

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

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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1974,6 +1974,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  };
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  };
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -57,31 +64,136 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
-// Creates a modified version of function definition that can be inserted at a
-// different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
-  auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-   FD->getSourceRange());
-  if (!CharRange)
+// Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
+// for global namespace, and endwith "::" otherwise.
+// Returns None if TargetNS is not a prefix of CurContext.
+llvm::Optional
+findContextForNS(llvm::StringRef TargetNS, const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  // If TargetNS is empty, it means global ns, which is translation unit.
+  if (TargetNS.empty()) {
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+return CurContext;
+  }
+  // Otherwise we need to drop any trailing namespaces from CurContext until
+  // we reach TargetNS.
+  std::string TargetContextNS =
+  CurContext->isNamespace()
+  ? llvm::cast(CurContext)->getQualifiedNameAsString()
+  : "";
+  TargetContextNS.append("::");
+
+  llvm::StringRef CurrentContextNS(TargetContextNS);
+  // If TargetNS is not a prefix of CurrentContext, there's no way to reach
+  // it.
+  if (!CurrentContextNS.startswith(TargetNS))
 return llvm::None;
+
+  while (CurrentContextNS != TargetNS) {
+CurContext = CurContext->getParent();
+// These colons always exists since Tar

[PATCH] D70535: [clangd] Define out-of-line qualify return value

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

merge_guards_bot added a comment.

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

  failed: Clangd Unit Tests._/ClangdTests/DefineOutlineTest.QualifyReturnValue

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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 231866.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Extract replacement applying part to a new function and add a fixme for 
sharing it with define inline code-action.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1974,6 +1974,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  };
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -57,31 +64,136 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
-// Creates a modified version of function definition that can be inserted at a
-// different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
-  auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-   FD->getSourceRange());
-  if (!CharRange)
+// Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
+// for global namespace, and endwith "::" otherwise.
+// Returns None if TargetNS is not a prefix of CurContext.
+llvm::Optional
+findContextForNS(llvm::StringRef TargetNS, const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  // If TargetNS is empty, it means global ns, which is translation unit.
+  if (TargetNS.empty()) {
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+return CurContext;
+  }
+  // Otherwise we need to drop any trailing namespaces from CurContext until
+  // we reach TargetNS.
+  std::string TargetContextNS =
+  CurContext->isNamespace()
+  ? llvm::cast(CurContext)->getQualifiedNameAsString()
+  : "";
+  TargetContextNS.append("::");
+
+  llvm::StringRef CurrentContextNS(TargetContextNS);
+  // If TargetNS is not a prefix of CurrentContext, there's no way to reach
+  // it.
+  if (!CurrentContextNS.startswith(TargetNS))
 re

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:153
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+  SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());

kadircet wrote:
> hokein wrote:
> > we have similar code in define-inline as well, would be nice to have them 
> > in a single place in the long term. probably a FIXME?
> they're quite similar but, different in nature. one of them returns the full 
> function definition, including template parameter lists, whereas the other 
> only operates on function body.
yes, the only difference is the range, right? the logic of applying 
replacements, getting correct begin/end offset, and getting the interesting 
content is the same. we could abstract a function that taking the range into 
the parameter.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(

kadircet wrote:
> hokein wrote:
> > oh, this is very tricky case (I think you meant to test the public 
> > members), note that Bar and foo are private member of Foo, we can't move 
> > the body out of the class `Foo`, for this case I think we should disallow 
> > the tweak.
> > 
> > No need to do it in this patch, but please update the test here (to test 
> > public members).
> I don't follow, the following compiles nicely:
> ```
> namespace a {
> class Foo {
>   class Bar {};
>   Bar foo();
> };
> }  // namespace a
> 
> a::Foo::Bar a::Foo::foo() { return {}; }
> ```
> 
> the problem here is we are not qualifying the function name, which is handled 
> in the follow up patch D70656
ah, you are right. I think I was somehow confused with the `a::Foo::Bar foo()`.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2007
+Bar foo() ;
+  }
+})cpp",

nit: ";"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:153
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+  SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());

hokein wrote:
> we have similar code in define-inline as well, would be nice to have them in 
> a single place in the long term. probably a FIXME?
they're quite similar but, different in nature. one of them returns the full 
function definition, including template parameter lists, whereas the other only 
operates on function body.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1778
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";

hokein wrote:
> can't we merge these into the above `ApplyTest`?
I would rather keep these separate, as these tests tends to get out of control 
otherwise, e.g. `Hover.All`



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(

hokein wrote:
> oh, this is very tricky case (I think you meant to test the public members), 
> note that Bar and foo are private member of Foo, we can't move the body out 
> of the class `Foo`, for this case I think we should disallow the tweak.
> 
> No need to do it in this patch, but please update the test here (to test 
> public members).
I don't follow, the following compiles nicely:
```
namespace a {
class Foo {
  class Bar {};
  Bar foo();
};
}  // namespace a

a::Foo::Bar a::Foo::foo() { return {}; }
```

the problem here is we are not qualifying the function name, which is handled 
in the follow up patch D70656


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 231511.
kadircet marked 9 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1974,6 +1974,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  }
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -57,31 +64,127 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
+// Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
+// for global namespace, and endwith "::" otherwise.
+// Returns None if TargetNS is not a prefix of CurContext.
+llvm::Optional
+findContextForNS(llvm::StringRef TargetNS, const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  // If TargetNS is empty, it means global ns, which is translation unit.
+  if (TargetNS.empty()) {
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+return CurContext;
+  }
+  // Otherwise we need to drop any trailing namespaces from CurContext until
+  // we reach TargetNS.
+  std::string TargetContextNS =
+  CurContext->isNamespace()
+  ? llvm::cast(CurContext)->getQualifiedNameAsString()
+  : "";
+  TargetContextNS.append("::");
+
+  llvm::StringRef CurrentContextNS(TargetContextNS);
+  // If TargetNS is not a prefix of CurrentContext, there's no way to reach
+  // it.
+  if (!CurrentContextNS.startswith(TargetNS))
+return llvm::None;
+
+  while (CurrentContextNS != TargetNS) {
+CurContext = CurContext->getParent();
+// These colons always exists since TargetNS is a prefix of
+// CurrentContextNS, it ends with "::" and they are not equal.
+CurrentContextNS = CurrentContextNS.take_front(
+CurrentContextNS.drop_back(2).rfind("::") + 2);
+  }
+  return CurContext;
+}
+
 // Creates a modified version of function definition that can be inserted at a
-// different location. Contains both function signature and body.

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:69
+llvm::Optional
+synthesizeContextForNS(llvm::StringRef TargetNS,
+   const DeclContext *CurContext) {

could you add some documentations, e.g. what's the requirement for the input 
`TargetNS`?

I'm not sure `Synthesize` is clear here, maybe `lookupTargetContext` or 
`findTargetContext`?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:77
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+  } else {

nit: maybe use early return?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:117
+  bool HadErrors = false;
+  tooling::Replacements Replacements;
+  findExplicitReferences(FD, [&](ReferenceLoc Ref) {

could we use a more meaningful name, maybe AddQualifierEdit? Would be nice to 
have some comments explaining what the following code does.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:149
+llvm::inconvertibleErrorCode(),
+"define outline: Failed to compute qualifiers see logs for details.");
+  }

nit: I think this error message is exposed to users, I'm not sure "see logs for 
details" is friendly to them.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:153
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+  SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());

we have similar code in define-inline as well, would be nice to have them in a 
single place in the long term. probably a FIXME?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:173
 
-  // FIXME: Qualify return type.
   // FIXME: Qualify function name depending on the target context.
 }

btw, do we need to qualify the function parameters. maybe it is not needed, but 
would be nice to have some brief comments explaining it.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:116
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
+llvm::Expected

could you also update the comments here, mentioning the function handles the 
return type qualifiers properly?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1778
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";

can't we merge these into the above `ApplyTest`?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(

oh, this is very tricky case (I think you meant to test the public members), 
note that Bar and foo are private member of Foo, we can't move the body out of 
the class `Foo`, for this case I think we should disallow the tweak.

No need to do it in this patch, but please update the test here (to test public 
members).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1974,6 +1974,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  }
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -57,31 +64,127 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
+// Synthesize a DeclContext for TargetNS from CurContext.
+llvm::Optional
+synthesizeContextForNS(llvm::StringRef TargetNS,
+   const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  if (TargetNS.empty()) {
+// If TargetNS is empty, it means global ns, which is translation unit.
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+  } else {
+// Otherwise we need to drop any trailing namespaces from CurContext until
+// we reach TargetNS.
+std::string TargetContextNS =
+CurContext->isNamespace()
+? llvm::cast(CurContext)->getQualifiedNameAsString()
+: "";
+TargetContextNS.append("::");
+
+llvm::StringRef CurrentContextNS(TargetContextNS);
+// If TargetNS is not a prefix of CurrentContext, there's no way to reach
+// it.
+if (!CurrentContextNS.startswith(TargetNS))
+  return llvm::None;
+
+while (CurrentContextNS != TargetNS) {
+  CurContext = CurContext->getParent();
+  // These colons always exists since TargetNS is a prefix of
+  // CurrentContextNS, it ends with "::" and they are not equal.
+  CurrentContextNS = CurrentContextNS.take_front(
+  CurrentContextNS.drop_back(2).rfind("::") + 2);
+}
+  }
+  return CurContext;
+}
+
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
+llvm::Expected
+getFunctionSourceCode(const FunctionDecl *FD, llvm:

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[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 
"627221d6588eb650759f39e80858abb2a3848ccb". 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/D70535/new/

https://reviews.llvm.org/D70535



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

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Move TargetContext generation logic to a separate function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1775,6 +1775,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  }
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -20,10 +24,13 @@
 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -68,29 +75,125 @@
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
+// Synthesize a DeclContext for TargetNS from CurContext.
+llvm::Optional
+synthesizeContextForNS(llvm::StringRef TargetNS,
+   const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  if (TargetNS.empty()) {
+// If TargetNS is empty, it means global ns, which is translation unit.
+while (!CurContext->isTranslationUnit())
+  CurContext = CurContext->getParent();
+  } else {
+// Otherwise we need to drop any trailing namespaces from CurContext until
+// we reach TargetNS.
+std::string TargetContextNS =
+CurContext->isNamespace()
+? llvm::cast(CurContext)->getQualifiedNameAsString()
+: "";
+TargetContextNS.append("::");
+
+llvm::StringRef CurrentContextNS(TargetContextNS);
+// If TargetNS is not a prefix of CurrentContext, there's no way to reach
+// it.
+if (!CurrentContextNS.startswith(TargetNS))
+  return llvm::None;
+
+while (CurrentContextNS != TargetNS) {
+  CurContext = CurContext->getParent();
+  // These colons always exists since TargetNS is a prefix of
+  // CurrentContextNS, it ends with "::" and they are not equal.
+  CurrentContextNS = CurrentContextNS.take_front(
+  CurrentContextNS.drop_back(2).rfind("::") + 2);
+}
+  }
+  return CurContext;
+}
+
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
+llvm::Expected

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535

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
@@ -1775,6 +1775,56 @@
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  }
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+llvm::errs() << EditedFiles.begin()->second << '\n';
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -22,8 +26,10 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -70,27 +76,89 @@
 
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
+llvm::Expected
+getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-   FD->getSourceRange());
-  if (!CharRange)
-return llvm::None;
-  CharRange->setBegin(getBeginLoc(FD));
+  const DeclContext *TargetContext = FD->getLexicalDeclContext();
+  while (!TargetContext->isTranslationUnit()) {
+if (TargetContext->isNamespace()) {
+  auto *NSD = llvm::dyn_cast(TargetContext);
+  if (NSD->getNameAsString() == TargetNamespace)
+break;
+}
+TargetContext = TargetContext->getLexicalParent();
+  }
+
+  bool HadErrors = false;
+  tooling::Replacements Replacements;
+  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())
+  return;
+
+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(), TargetCo

[PATCH] D70535: [clangd] Define out-of-line qualify return value

[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 added a parent revision: D69298: [clangd] Define out-of-line initial 
apply logic.

Return type might need qualification if insertion context doesn't have
the same decls visible as the source context.

This patch adds qualification for return value to cover such cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70535

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
@@ -1761,6 +1761,57 @@
"template <> void foo() { return; }")));
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+namespace a { class Foo; }
+using namespace a;
+Foo foo() ;)cpp",
+   "a::Foo foo() { return; }"},
+  {R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar fo^o() { return {}; }
+  }
+})cpp",
+   R"cpp(
+namespace a {
+  class Foo {
+class Bar {};
+Bar foo() ;
+  }
+})cpp",
+   "a::Foo::Bar foo() { return {}; }\n"},
+  {R"cpp(
+class Foo;
+Foo fo^o() { return; })cpp",
+   R"cpp(
+class Foo;
+Foo foo() ;)cpp",
+   "Foo foo() { return; }"},
+  };
+  llvm::StringMap EditedFiles;
+  for (auto &Case : Cases) {
+EditedFiles.clear();
+apply(Case.Test, &EditedFiles);
+EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+llvm::errs() << EditedFiles.begin()->second << '\n';
+EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
 } // 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
@@ -6,13 +6,17 @@
 //
 //===--===//
 
+#include "AST.h"
+#include "FindTarget.h"
 #include "HeaderSourceSwitch.h"
+#include "Logger.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"
@@ -22,8 +26,10 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -70,27 +76,89 @@
 
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
-llvm::Optional getFunctionSourceCode(const FunctionDecl *FD) {
+llvm::Expected
+getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-   FD->getSourceRange());
-  if (!CharRange)
-return llvm::None;
-  CharRange->setBegin(getBeginLoc(FD));
+  const DeclContext *TargetContext = FD->getLexicalDeclContext();
+  while (!TargetContext->isTranslationUnit()) {
+if (TargetContext->isNamespace()) {
+  auto *NSD = llvm::dyn_cast(TargetContext);
+  if (NSD->getNameAsString() == TargetNamespace)
+break;
+}
+TargetContext = TargetContext->getLexicalParent();
+  }
+
+  bool HadErrors = false;
+  tooling::Replacements Replacements;
+  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())
+  return;
+
+for (const Na