[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

ApplyTest fails on Windows; probably the usual delayed template parsing thing: 
http://45.33.8.238/win/3368/step_7.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGce2189202245: [clangd] Define out-of-line initial apply 
logic (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.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
@@ -1868,6 +1868,110 @@
 })cpp");
 }
 
+TEST_F(DefineOutlineTest, FailsWithoutSource) {
+  FileName = "Test.hpp";
+  llvm::StringRef Test = "void fo^o() { return; }";
+  llvm::StringRef Expected =
+  "fail: Couldn't find a suitable implementation file.";
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DefineOutlineTest, ApplyTest) {
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Simple check
+  {
+  "void fo^o() { return; }",
+  "void foo() ;",
+  "void foo() { return; }",
+  },
+  // Templated function.
+  {
+  "template  void fo^o(T, T x) { return; }",
+  "template  void foo(T, T x) ;",
+  "template  void foo(T, T x) { return; }",
+  },
+  {
+  "template  void fo^o() { return; }",
+  "template  void foo() ;",
+  "template  void foo() { return; }",
+  },
+  // Template specialization.
+  {
+  R"cpp(
+template  void foo();
+template <> void fo^o() { return; })cpp",
+  R"cpp(
+template  void foo();
+template <> void foo() ;)cpp",
+  "template <> void foo() { return; }",
+  },
+  };
+  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)));
+  }
+}
+
+TEST_F(DefineOutlineTest, HandleMacros) {
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+  #define BODY { return; }
+  void f^oo()BODY)cpp",
+   R"cpp(
+  #define BODY { return; }
+  void foo();)cpp",
+   "void foo()BODY"},
+
+  {R"cpp(
+  #define BODY return;
+  void f^oo(){BODY})cpp",
+   R"cpp(
+  #define BODY return;
+  void foo();)cpp",
+   "void foo(){BODY}"},
+
+  {R"cpp(
+  #define TARGET void foo()
+  [[TARGET]]{ return; })cpp",
+   R"cpp(
+  #define TARGET void foo()
+  TARGET;)cpp",
+   "TARGET{ return; }"},
+
+  {R"cpp(
+  #define TARGET foo
+  void [[TARGET]](){ return; })cpp",
+   R"cpp(
+  #define TARGET foo
+  void TARGET();)cpp",
+   "void TARGET(){ return; }"},
+  };
+  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)));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -127,7 +127,7 @@
 ADD_FAILURE() << "There were changes to additional files, but client "
  "provided a nullptr for EditedFiles.";
   else
-EditedFiles->try_emplace(It.first(), Unwrapped.str());
+EditedFiles->insert_or_assign(It.first(), Unwrapped.str());
 }
   }
   return EditedMainFile;
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
@@ -13,9 +13,17 @@
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "cla

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[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 
"6a2d56e54fa4a2009787a605607b0df7fe16dd98". 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/D69298/new/

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 231382.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments
- Better handling for macros and tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.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
@@ -1870,6 +1870,110 @@
 })cpp");
 }
 
+TEST_F(DefineOutlineTest, FailsWithoutSource) {
+  FileName = "Test.hpp";
+  llvm::StringRef Test = "void fo^o() { return; }";
+  llvm::StringRef Expected =
+  "fail: Couldn't find a suitable implementation file.";
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DefineOutlineTest, ApplyTest) {
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Simple check
+  {
+  "void fo^o() { return; }",
+  "void foo() ;",
+  "void foo() { return; }",
+  },
+  // Templated function.
+  {
+  "template  void fo^o(T, T x) { return; }",
+  "template  void foo(T, T x) ;",
+  "template  void foo(T, T x) { return; }",
+  },
+  {
+  "template  void fo^o() { return; }",
+  "template  void foo() ;",
+  "template  void foo() { return; }",
+  },
+  // Template specialization.
+  {
+  R"cpp(
+template  void foo();
+template <> void fo^o() { return; })cpp",
+  R"cpp(
+template  void foo();
+template <> void foo() ;)cpp",
+  "template <> void foo() { return; }",
+  },
+  };
+  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)));
+  }
+}
+
+TEST_F(DefineOutlineTest, HandleMacros) {
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  {R"cpp(
+  #define BODY { return; }
+  void f^oo()BODY)cpp",
+   R"cpp(
+  #define BODY { return; }
+  void foo();)cpp",
+   "void foo()BODY"},
+
+  {R"cpp(
+  #define BODY return;
+  void f^oo(){BODY})cpp",
+   R"cpp(
+  #define BODY return;
+  void foo();)cpp",
+   "void foo(){BODY}"},
+
+  {R"cpp(
+  #define TARGET void foo()
+  [[TARGET]]{ return; })cpp",
+   R"cpp(
+  #define TARGET void foo()
+  TARGET;)cpp",
+   "TARGET{ return; }"},
+
+  {R"cpp(
+  #define TARGET foo
+  void [[TARGET]](){ return; })cpp",
+   R"cpp(
+  #define TARGET foo
+  void TARGET();)cpp",
+   "void TARGET(){ return; }"},
+  };
+  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)));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -127,7 +127,7 @@
 ADD_FAILURE() << "There were changes to additional files, but client "
  "provided a nullptr for EditedFiles.";
   else
-EditedFiles->try_emplace(It.first(), Unwrapped.str());
+EditedFiles->insert_or_assign(It.first(), Unwrapped.str());
 }
   }
   return EditedMainFile;
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
@@ -13,9 +13,17 @@
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "clang/T

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

looks good.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:53
+// template keyword for templated functions.
+// FIXME: This is shared with define inline, move them to a common header once
+// we have a place for such.

this function seems trivial, I'd inline them into call sides for both define 
inline and outline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Get rid of raw string literals inside macros


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.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
@@ -1722,6 +1722,59 @@
 };)cpp");
 }
 
+TEST_F(DefineOutlineTest, FailsWithoutSource) {
+  FileName = "Test.hpp";
+  llvm::StringRef Test = "void fo^o() { return; }";
+  llvm::StringRef Expected =
+  "fail: Couldn't find a suitable implementation file.";
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DefineOutlineTest, ApplyTest) {
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedHeader;
+llvm::StringRef ExpectedSource;
+  } Cases[] = {
+  // Simple check
+  {
+  "void fo^o() { return; }",
+  "void foo() ;",
+  "void foo() { return; }",
+  },
+  // Templated function.
+  {
+  "template  void fo^o(T, T x) { return; }",
+  "template  void foo(T, T x) ;",
+  "template  void foo(T, T x) { return; }",
+  },
+  {
+  "template  void fo^o() { return; }",
+  "template  void foo() ;",
+  "template  void foo() { return; }",
+  },
+  // Template specialization.
+  {
+  R"cpp(
+template  void foo();
+template <> void fo^o() { return; })cpp",
+  R"cpp(
+template  void foo();
+template <> void foo() ;)cpp",
+  "template <> void foo() { return; }",
+  },
+  };
+  for (const auto &Case : Cases) {
+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/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -127,7 +127,7 @@
 ADD_FAILURE() << "There were changes to additional files, but client "
  "provided a nullptr for EditedFiles.";
   else
-EditedFiles->try_emplace(It.first(), Unwrapped.str());
+EditedFiles->insert_or_assign(It.first(), Unwrapped.str());
 }
   }
   return EditedMainFile;
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
@@ -9,12 +9,21 @@
 #include "HeaderSourceSwitch.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/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -39,6 +48,60 @@
   return nullptr;
 }
 
+// Returns the begining location for a FunctionDecl. Returns location of
+// template keyword for templated functions.
+// FIXME: This is shared with define inline, move them to a common header once
+// we have a place for such.
+const SourceLocation getBeginLoc(const FunctionDecl *FD) {
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();
+  return FD->getBeginLoc();
+}
+
+llvm::Optional getSourceFile(llvm::StringRef FileName,
+   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+  FileName,
+  &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+return *Source;
+  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().getSourceMana

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

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
@@ -1644,6 +1644,45 @@
 };)cpp");
 }
 
+TEST_F(DefineOutlineTest, ApplyTest) {
+  FileName = "Test.hpp";
+
+  // No implementation file.
+  EXPECT_EQ(apply("void fo^o() { return; }"),
+"fail: Couldn't find a suitable implementation file.");
+
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("Test.cpp"),
+"void foo() { return; }")));
+
+  EditedFiles.clear();
+  // Templated function.
+  EXPECT_EQ(apply("template  void fo^o(T, T x) { return; }",
+  &EditedFiles),
+"template  void foo(T, T x) ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(
+  testPath("Test.cpp"),
+  "template  void foo(T, T x) { return; }")));
+
+  EditedFiles.clear();
+  // Template specialization.
+  EXPECT_EQ(apply(R"cpp(
+template  void foo();
+template <> void fo^o() { return; })cpp",
+  &EditedFiles),
+R"cpp(
+template  void foo();
+template <> void foo() ;)cpp");
+  EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+   testPath("Test.cpp"),
+   "template <> void foo() { return; }")));
+}
+
 } // 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
@@ -9,12 +9,21 @@
 #include "HeaderSourceSwitch.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/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -39,6 +48,60 @@
   return nullptr;
 }
 
+// Returns the begining location for a FunctionDecl. Returns location of
+// template keyword for templated functions.
+// FIXME: This is shared with define inline, move them to a common header once
+// we have a place for such.
+const SourceLocation getBeginLoc(const FunctionDecl *FD) {
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();
+  return FD->getBeginLoc();
+}
+
+llvm::Optional getSourceFile(llvm::StringRef FileName,
+   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+  FileName,
+  &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+return *Source;
+  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)
+return llvm::None;
+  CharRange->setBegin(getBeginLoc(FD));
+
+  // FIXME: Qualify return type.
+  // FIXME: Qualify function name depending on the target context.
+  return toSourceCode(SM, *CharRange);
+}
+
+// Returns the most natural insertion point for \p QualifiedName in \p Contents.
+// This currently cares about only the namespace proximity, but in feature it
+// should also try to follow ordering of declarations. For example, if decls
+// come in order `foo, bar, baz` then this function should return some point
+// between foo and baz for inserting bar.
+llvm::Expected 

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:55
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();

hokein wrote:
> Could you confirm the code handle template specializations as well? I think 
> `getDescribedFunctionTemplate` will return null when FD is a specialization, 
> and we still miss the template list.
added a unittest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:55
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();

Could you confirm the code handle template specializations as well? I think 
`getDescribedFunctionTemplate` will return null when FD is a specialization, 
and we still miss the template list.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:73
+  auto &SM = FD->getASTContext().getSourceManager();
+  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
+   FD->getSourceRange());

I think we could simplify the code like:

```
const auto* TargetFD = FD->getDescribedFunctionTemplate() ? 
FD->getDescribedFunctionTemplate(): FD;
auto CharRange = toHaleOpenFileRange(.., FD->getSourceRange());
```



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:131
+llvm::StringRef FileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+

should we use `getCanonicalPath` in the `SourceCode.h`?



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:158
+auto InsertionPoint = Region.EligiblePoints.back();
+auto InsertionOffset = positionToOffset(Contents, InsertionPoint);
+if (!InsertionOffset)

nit: maybe put the code for calculating the insertion point to a separate 
function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 226134.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments
- Handle template parameters when copying function and add tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

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
@@ -1562,6 +1562,31 @@
 })cpp");
 }
 
+TEST_F(DefineOutlineTest, ApplyTest) {
+  FileName = "Test.hpp";
+
+  // No implementation file.
+  EXPECT_EQ(apply("void fo^o() { return; }"),
+"fail: Couldn't find a suitable implementation file.");
+
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("Test.cpp"),
+"void foo() { return; }")));
+
+  EditedFiles.clear();
+  EXPECT_EQ(apply("template  void fo^o(T, T x) { return; }",
+  &EditedFiles),
+"template  void foo(T, T x) ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(
+  testPath("Test.cpp"),
+  "template  void foo(T, T x) { return; }")));
+}
+
 } // 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
@@ -9,12 +9,19 @@
 #include "HeaderSourceSwitch.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/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
@@ -39,6 +46,41 @@
   return nullptr;
 }
 
+// Returns the begining location for a FunctionDecl. Returns location of
+// template keyword for templated functions.
+// FIXME: This is shared with define inline, move them to a common header once
+// we have a place for such.
+const SourceLocation getBeginLoc(const FunctionDecl *FD) {
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+return FTD->getBeginLoc();
+  return FD->getBeginLoc();
+}
+
+llvm::Optional getSourceFile(llvm::StringRef FileName,
+   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+  FileName,
+  &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+return *Source;
+  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)
+return llvm::None;
+  CharRange->setBegin(getBeginLoc(FD));
+
+  // FIXME: Qualify return type.
+  // FIXME: Qualify function name depending on the target context.
+  return toSourceCode(SM, *CharRange);
+}
+
 /// Moves definition of a function/method to an appropriate implementation file.
 ///
 /// Before:
@@ -84,8 +126,66 @@
   }
 
   Expected apply(const Selection &Sel) override {
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Not implemented yet");
+const SourceManager &SM = Sel.AST.getSourceManager();
+llvm::StringRef FileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+
+auto CCFile = getSourceFile(FileName, Sel);
+if (!CCFile)
+  return llvm::createStringError(
+  llvm::inconvertibleErrorCode(),
+  "Couldn't find a suitable implementation file.");
+
+auto &FS =
+Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
+auto Buffer = FS.getBufferForFile(*CCFile);
+// FIXME: Maybe we should consider creating the implementation file if it
+//

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 7 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1574
+  ExtraFiles["Test.cpp"] = "";
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,

hokein wrote:
> thinking more about this,  if this function is inline, we will leave an 
> unnecessary `inline` keyword after running the code action. No need to 
> address it in the patch, just keep in mind.
> 
> can we add more tests? e.g. template functions.
added a fixme for inline stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:59
+// different location. Contains both function signature and body.
+llvm::Optional moveFunctionDef(const FunctionDecl *FD) {
+  auto &SM = FD->getASTContext().getSourceManager();

the function name doesn't reflect what it does, it doesn't move the function, 
it just returns the source code of the function, I'd call it some name like 
`getFunctionSourceCode`.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:117
+const SourceManager &SM = Sel.AST.getSourceManager();
+llvm::StringRef FileName = SM.getFilename(Sel.Cursor);
+

The FileName is not always absolute, `getCorrespondingHeaderOrSource` is 
expecting an absolute file path input.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:119
+
+auto SourceFile = getSourceFile(FileName, Sel);
+if (!SourceFile)

could we use a better name, `Source` in the context here has two different 
meaning: 1) the corresponding .cc file 2) the source of moving function 
declaration (we called it `Source`);

Maybe call it `CCFile`?




Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:157
+SMFF.get().getComposedLoc(SMFF.get().getMainFileID(), 
*InsertionOffset);
+const tooling::Replacement InsertFunctionDef(SMFF.get(), InsertLoc, 0,
+ *FuncDef);

maybe just `const tooling::Replacement InsertFunctionDef(Contents, 
*InsertionOffset, 0, *FunDef);`, which would save us a `InsertLoc`.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:166
+Sel.AST.getSourceManager(),
+CharSourceRange(Source->getBody()->getSourceRange(), /*ITR=*/true),
+";");

nit: I think 
`CharSourceRange::getCharRange(Source->getBody()->getSourceRange())` is clearer.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1574
+  ExtraFiles["Test.cpp"] = "";
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,

thinking more about this,  if this function is inline, we will leave an 
unnecessary `inline` keyword after running the code action. No need to address 
it in the patch, just keep in mind.

can we add more tests? e.g. template functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298



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

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69298

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
@@ -1562,6 +1562,21 @@
 })cpp");
 }
 
+TEST_F(DefineOutlineTest, ApplyTest) {
+  FileName = "Test.hpp";
+
+  // No implementation file.
+  EXPECT_EQ(apply("void fo^o() { return; }"),
+"fail: Couldn't find a suitable implementation file.");
+
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("Test.cpp"),
+"void foo() { return; }")));
+}
+
 } // 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
@@ -9,12 +9,18 @@
 #include "HeaderSourceSwitch.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/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
@@ -39,6 +45,29 @@
   return nullptr;
 }
 
+llvm::Optional getSourceFile(llvm::StringRef FileName,
+   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+  FileName,
+  &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+return *Source;
+  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 moveFunctionDef(const FunctionDecl *FD) {
+  auto &SM = FD->getASTContext().getSourceManager();
+  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
+   FD->getSourceRange());
+  if (!CharRange)
+return llvm::None;
+
+  // FIXME: Qualify return type.
+  // FIXME: Qualify function name depending on the target context.
+  return toSourceCode(SM, *CharRange);
+}
+
 /// Moves definition of a function/method to an appropriate implementation file.
 ///
 /// Before:
@@ -84,8 +113,66 @@
   }
 
   Expected apply(const Selection &Sel) override {
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Not implemented yet");
+const SourceManager &SM = Sel.AST.getSourceManager();
+llvm::StringRef FileName = SM.getFilename(Sel.Cursor);
+
+auto SourceFile = getSourceFile(FileName, Sel);
+if (!SourceFile)
+  return llvm::createStringError(
+  llvm::inconvertibleErrorCode(),
+  "Couldn't find a suitable implementation file.");
+
+auto &FS =
+Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
+auto Buffer = FS.getBufferForFile(*SourceFile);
+// FIXME: Maybe we should consider creating the implementation file if it
+// doesn't exist?
+if (!Buffer)
+  return llvm::createStringError(Buffer.getError(),
+ Buffer.getError().message());
+auto Contents = Buffer->get()->getBuffer();
+auto Region =
+getEligiblePoints(Contents, Source->getQualifiedNameAsString(),
+  getFormatStyleForFile(*SourceFile, Contents, &FS));
+
+assert(!Region.EligiblePoints.empty());
+// FIXME: This selection can be made smarter by looking at the definition
+// locations for adjacent decls to Source. Unfortunately psudeo parsing in
+// getEligibleRegions only knows about namespace begin/end events so we
+// can't match function start/end positions yet.
+auto InsertionPoint = Region.EligiblePoints.back();
+auto InsertionOffset = positionToOffset(Contents, InsertionPoint);
+if (!InsertionOffset)
+  return InsertionOffset.takeError();
+
+auto FuncDef = moveFunctionDef(Source);
+if (!FuncDef)
+  return llvm::createStringError(
+  llvm::incon

[PATCH] D69298: [clangd] Define out-of-line initial apply logic

[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.

Initial implementation for apply logic, replaces function body with a
semicolon in source location and copies the full function definition into target
location.

Will handle qualification of return type and function name in following patches
to keep the changes small.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69298

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
@@ -1561,6 +1561,21 @@
 })cpp");
 }
 
+TEST_F(DefineOutlineTest, ApplyTest) {
+  FileName = "Test.hpp";
+
+  // No implementation file.
+  EXPECT_EQ(apply("void fo^o() { return; }"),
+"fail: Couldn't find a suitable implementation file.");
+
+  llvm::StringMap EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  EXPECT_EQ(apply("void fo^o() { return; }", &EditedFiles), "void foo() ;");
+  EXPECT_THAT(EditedFiles,
+  testing::ElementsAre(FileWithContents(testPath("Test.cpp"),
+"void foo() { return; }")));
+}
+
 } // 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
@@ -9,14 +9,18 @@
 #include "HeaderSourceSwitch.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/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Driver/Types.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
@@ -40,6 +44,29 @@
   return nullptr;
 }
 
+llvm::Optional getSourceFile(llvm::StringRef FileName,
+   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+  FileName,
+  &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+return *Source;
+  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 moveFunctionDef(const FunctionDecl *FD) {
+  auto &SM = FD->getASTContext().getSourceManager();
+  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
+   FD->getSourceRange());
+  if (!CharRange)
+return llvm::None;
+
+  // FIXME: Qualify return type.
+  // FIXME: Qualify function name depending on the target context.
+  return toSourceCode(SM, *CharRange);
+}
+
 /// Moves definition of a function/method to an appropriate implementation file.
 /// If current file is already an implementation file, tries to move the
 /// definition out-of-line.
@@ -70,7 +97,7 @@
 
   bool prepare(const Selection &Sel) override {
 const SourceManager &SM = Sel.AST.getSourceManager();
-llvm::StringRef FileName = SM.getFilename(Sel.Cursor);
+FileName = SM.getFilename(Sel.Cursor);
 
 // Bail out if we are not in a header file.
 // FIXME: We might want to consider moving method definitions below class
@@ -93,12 +120,68 @@
   }
 
   Expected apply(const Selection &Sel) override {
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "Not implemented yet");
+auto SourceFile = getSourceFile(FileName, Sel);
+if (!SourceFile)
+  return llvm::createStringError(
+  llvm::inconvertibleErrorCode(),
+  "Couldn't find a suitable implementation file.");
+
+auto &FS =
+Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
+auto Buffer = FS.getBufferForFile(*SourceFile);
+// FIXME: Maybe we should consider creating the implementation file if it
+// doesn't exist?
+if (!Buffer)
+  return llvm::createStringError(Buffer.getError(),
+ Buffer.getError().message());
+auto Contents = Buffer->get()->getBuffer();
+auto Region =
+getEligiblePoints(Contents, Source->getQualifiedNameAsString(),
+  getFormatStyleForFile(*SourceFile, Contents, &FS));
+
+assert(!Region.EligiblePoints.empty(