[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS created this revision.
FabioRS added a reviewer: sammccall.
FabioRS added a project: clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
FabioRS requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.

I miss more automatically refactoring functions when working with already 
running code, so I am making some small addition that I hope help more people.

This works by checking if the function is a method (CXXMethodDecl), then 
collecting information about the function that the code is being extracted, 
looking for the declaration if it is out-of-line, creating the declaration if 
it is necessary and putting the extracted function as a class-method.

This is my first code review request, sorry if I did something wrong.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122698

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -114,16 +114,48 @@
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
-  // We don't extract from methods for now since they may involve multi-file
-  // edits
-  std::string MethodFailInput = R"cpp(
+  std::string MethodInput = R"cpp(
 class T {
   void f() {
 [[int x;]]
   }
 };
   )cpp";
-  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+  std::string MethodCheckOutput = R"cpp(
+class T {
+  void extracted() {
+int x;
+}
+void f() {
+extracted();
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodInput), MethodCheckOutput);
+
+  std::string OutOfLineMethodInput = R"cpp(
+class T {
+  void f();
+};
+
+void T::f() {
+  [[int x;]]
+}
+  )cpp";
+  std::string OutOfLineMethodCheckOutput = R"cpp(
+class T {
+  void extracted();
+void f();
+};
+
+void T::extracted() {
+int x;
+}
+void T::f() {
+  extracted();
+}
+  )cpp";
+  EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput);
 
   // We don't extract from templated functions for now as templates are hard
   // to deal with.
@@ -159,6 +191,117 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f();
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted();
+void f();
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() const {
+int x;
+}
+void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted() const;
+void f() const;
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+  << "The header should be edited and receives the declaration of the new "
+ "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, StaticDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  static void f();
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  static void extracted();
+static void f();
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
 TEST_F

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS updated this revision to Diff 419290.
FabioRS marked 14 inline comments as done.
FabioRS added a comment.

I am not sure about the LangOptions object, the NestedNameSpecifier needs it to 
correctly print the nested classes-name and not sure about the getEnclosing() 
too.

I can not run the test the consteval, so it is not in the diff.

  TestTU failed to build (suppress with /*error-ok*/): 
  [1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, fixes: 
{change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}
  
  For code:
  
  consteval void SomeClass::f() const {
int x;
  }


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

https://reviews.llvm.org/D122698

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -226,6 +226,87 @@
   EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
 }
 
+TEST_F(ExtractFunctionTest, DifferentFilesNestedTest) {
+  Header = R"cpp(
+class T {
+class SomeClass {
+  void f();
+};
+};
+  )cpp";
+
+  std::string NestedOutOfLineSource = R"cpp(
+void T::SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string NestedOutOfLineSourceOutputCheck = R"cpp(
+void T::SomeClass::extracted() {
+int x;
+}
+void T::SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string NestedHeaderOutputCheck = R"cpp(
+class T {
+class SomeClass {
+  void extracted();
+void f();
+};
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(NestedOutOfLineSource, &EditedFiles),
+NestedOutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, NestedHeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstexprDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  constexpr void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+constexpr void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+constexpr void SomeClass::extracted() const {
+int x;
+}
+constexpr void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  constexpr void extracted() const;
+constexpr void f() const;
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+  << "The header should be edited and receives the declaration of the new "
+ "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
 TEST_F(ExtractFunctionTest, ConstDifferentHeaderSourceTest) {
   Header = R"cpp(
 class SomeClass {
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -56,6 +56,8 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/LangOptions.h"
@@ -71,6 +73,8 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/raw_os_ostream.h"
 
 namespace clang {
 namespace clangd {
@@ -88,6 +92,12 @@
   OutsideFunc // Outside EnclosingFunction.
 };
 
+enum FunctionDeclKind {
+  InlineDefinition,
+  ForwardDeclaration,
+  OutOfLineDefinition
+};
+
 // A RootStmt is a statement that's fully selected including all it's children
 // and it's parent is unselected.
 // Check if a node is a root statement.
@@ -340,45 +350,62 @@
   QualType ReturnType;
   std::vector Parameters;
   SourceRange BodyRange;
-  SourceLocation InsertionPoint;
-  llvm::Optional DeclarationPoint;
-  llvm::Optional ParentContext;
-  const DeclContext *EnclosingFuncContext;
+  SourceLocation DefinitionPoint;
+  llvm::Optional ForwardDeclarationPoint;
+  llvm::Optional EnclosingClass;
+  NestedNameSpecifier *NestedNameSpec = nullptr;
+  const DeclContext *EnclosingFuncContext = nullptr;
   bool CallerReturnsValue = false;
   bool Static = false;
-  bool Constexpr = false;
-  bool OutOfLine = false;
-  bool ContextConst = false;
+  ConstexprSpecKind Constexpr = ConstexprSpecKind::Unspecified;
+  boo

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS updated this revision to Diff 419296.
FabioRS added a comment.

Full patch


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

https://reviews.llvm.org/D122698

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -114,16 +114,48 @@
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
-  // We don't extract from methods for now since they may involve multi-file
-  // edits
-  std::string MethodFailInput = R"cpp(
+  std::string MethodInput = R"cpp(
 class T {
   void f() {
 [[int x;]]
   }
 };
   )cpp";
-  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+  std::string MethodCheckOutput = R"cpp(
+class T {
+  void extracted() {
+int x;
+}
+void f() {
+extracted();
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodInput), MethodCheckOutput);
+
+  std::string OutOfLineMethodInput = R"cpp(
+class T {
+  void f();
+};
+
+void T::f() {
+  [[int x;]]
+}
+  )cpp";
+  std::string OutOfLineMethodCheckOutput = R"cpp(
+class T {
+  void extracted();
+void f();
+};
+
+void T::extracted() {
+int x;
+}
+void T::f() {
+  extracted();
+}
+  )cpp";
+  EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput);
 
   // We don't extract from templated functions for now as templates are hard
   // to deal with.
@@ -159,6 +191,198 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f();
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted();
+void f();
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, DifferentFilesNestedTest) {
+  Header = R"cpp(
+class T {
+class SomeClass {
+  void f();
+};
+};
+  )cpp";
+
+  std::string NestedOutOfLineSource = R"cpp(
+void T::SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string NestedOutOfLineSourceOutputCheck = R"cpp(
+void T::SomeClass::extracted() {
+int x;
+}
+void T::SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string NestedHeaderOutputCheck = R"cpp(
+class T {
+class SomeClass {
+  void extracted();
+void f();
+};
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(NestedOutOfLineSource, &EditedFiles),
+NestedOutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, NestedHeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstexprDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  constexpr void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+constexpr void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+constexpr void SomeClass::extracted() const {
+int x;
+}
+constexpr void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  constexpr void extracted() const;
+constexpr void f() const;
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+  << "The header should be edited and receives the declaration of the new "
+ "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, ConstDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() const {
+int x;
+}
+void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted() const;
+void f() const;
+};
+  )cpp";
+
+  llvm::StringMap Edited

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS added a comment.

In D122698#3418018 , @sammccall wrote:

> Thanks! I'll look at the changes in the morning, just wanted to mention one 
> thing
>
> In D122698#3417946 , @FabioRS wrote:
>
>> I can not run the test the consteval, so it is not in the diff.
>>
>>   TestTU failed to build (suppress with /*error-ok*/): 
>>   [1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, 
>> fixes: {change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}
>
> It's a c++20 feature, so you'll need to add something like 
> `ExtraArgs.push_back("-std=c++20");`.
>
> Though if there's no real difference between handling constexpr vs consteval, 
> up to you whether it's worth a separate test.

Thank you!
I think it is interesting to have the test to keep the consteval branch 
covered. I think maybe there will be more things to change, so I can send the 
test case for consteval tomorrow with the others changes.




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:455
+  return std::string(llvm::formatv("{0}{1} {2}({3}){4}", renderAttributes(),
+   printType(ReturnType, 
*EnclosingFuncContext),
+   Name, renderParametersForDefinition(),

sammccall wrote:
> For out-of-line defined methods, this is the wrong DC, which may lead to the 
> type being incorrectly qualified. The DC should rather be the enclosing class.
> 
> Similarly, renderParametersForDefinition uses EnclosingFuncContext too - 
> instead it should take the EnclosingFuncContext as a parameter. (And be 
> renamed `renderParametersForDeclaration`, since it's no longer just for 
> definitions)
It is only for out-of-line methods? I put this verification in the 
getEnclosing() of the new diff.



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

https://reviews.llvm.org/D122698

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

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS updated this revision to Diff 419616.
FabioRS marked 6 inline comments as done.
FabioRS set the repository for this revision to rG LLVM Github Monorepo.
FabioRS added a comment.

  namespace ns { int y = 1; void foo(); }
  void ns::foo() {[[int x; y++; ]]}



  namespace ns { int y = 1; void foo(); }
  void extracted() {
  int x; y++;
  }
  void ns::foo() {extracted(); }

This is not working, but I think it's not the scope of this review request, 
what do you think?

(See the tests line 478)

  class SomeClass {
static ns::A::C::RType extracted();
  static C::RType f();
  };

This is valid code but the syntactic looks strange with all the 
namespaces/classes, I tested various contexts but was not fixed, do you have 
some ideia for me to do?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122698

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -114,16 +114,48 @@
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
-  // We don't extract from methods for now since they may involve multi-file
-  // edits
-  std::string MethodFailInput = R"cpp(
+  std::string MethodInput = R"cpp(
 class T {
   void f() {
 [[int x;]]
   }
 };
   )cpp";
-  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+  std::string MethodCheckOutput = R"cpp(
+class T {
+  void extracted() {
+int x;
+}
+void f() {
+extracted();
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodInput), MethodCheckOutput);
+
+  std::string OutOfLineMethodInput = R"cpp(
+class T {
+  void f();
+};
+
+void T::f() {
+  [[int x;]]
+}
+  )cpp";
+  std::string OutOfLineMethodCheckOutput = R"cpp(
+class T {
+  void extracted();
+void f();
+};
+
+void T::extracted() {
+int x;
+}
+void T::f() {
+  extracted();
+}
+  )cpp";
+  EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput);
 
   // We don't extract from templated functions for now as templates are hard
   // to deal with.
@@ -159,6 +191,305 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f();
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted();
+void f();
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, DifferentFilesNestedTest) {
+  Header = R"cpp(
+class T {
+class SomeClass {
+  void f();
+};
+};
+  )cpp";
+
+  std::string NestedOutOfLineSource = R"cpp(
+void T::SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string NestedOutOfLineSourceOutputCheck = R"cpp(
+void T::SomeClass::extracted() {
+int x;
+}
+void T::SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string NestedHeaderOutputCheck = R"cpp(
+class T {
+class SomeClass {
+  void extracted();
+void f();
+};
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(NestedOutOfLineSource, &EditedFiles),
+NestedOutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, NestedHeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstexprDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  constexpr void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+constexpr void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+constexpr void SomeClass::extracted() const {
+int x;
+}
+constexpr void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  constexpr void extracted() const;
+constexpr void f() const;
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+  << "The header should be edited and receives the declaration of the

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS updated this revision to Diff 419868.
FabioRS marked an inline comment as done.
FabioRS added a comment.

I did some simple refactor on the code I did for the function-method and it 
fixed the namespace syntactic problem too. I am not sure if it expands the pull 
request scope.


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

https://reviews.llvm.org/D122698

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -114,16 +114,48 @@
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
-  // We don't extract from methods for now since they may involve multi-file
-  // edits
-  std::string MethodFailInput = R"cpp(
+  std::string MethodInput = R"cpp(
 class T {
   void f() {
 [[int x;]]
   }
 };
   )cpp";
-  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+  std::string MethodCheckOutput = R"cpp(
+class T {
+  void extracted() {
+int x;
+}
+void f() {
+extracted();
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodInput), MethodCheckOutput);
+
+  std::string OutOfLineMethodInput = R"cpp(
+class T {
+  void f();
+};
+
+void T::f() {
+  [[int x;]]
+}
+  )cpp";
+  std::string OutOfLineMethodCheckOutput = R"cpp(
+class T {
+  void extracted();
+void f();
+};
+
+void T::extracted() {
+int x;
+}
+void T::f() {
+  extracted();
+}
+  )cpp";
+  EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput);
 
   // We don't extract from templated functions for now as templates are hard
   // to deal with.
@@ -159,6 +191,333 @@
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  void f();
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+void SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  void extracted();
+void f();
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, DifferentFilesNestedTest) {
+  Header = R"cpp(
+class T {
+class SomeClass {
+  void f();
+};
+};
+  )cpp";
+
+  std::string NestedOutOfLineSource = R"cpp(
+void T::SomeClass::f() {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string NestedOutOfLineSourceOutputCheck = R"cpp(
+void T::SomeClass::extracted() {
+int x;
+}
+void T::SomeClass::f() {
+  extracted();
+}
+  )cpp";
+
+  std::string NestedHeaderOutputCheck = R"cpp(
+class T {
+class SomeClass {
+  void extracted();
+void f();
+};
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(NestedOutOfLineSource, &EditedFiles),
+NestedOutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, NestedHeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstexprDifferentHeaderSourceTest) {
+  Header = R"cpp(
+class SomeClass {
+  constexpr void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+constexpr void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+constexpr void SomeClass::extracted() const {
+int x;
+}
+constexpr void SomeClass::f() const {
+  extracted();
+}
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+class SomeClass {
+  constexpr void extracted() const;
+constexpr void f() const;
+};
+  )cpp";
+
+  llvm::StringMap EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+  << "The header should be edited and receives the declaration of the new "
+ "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, ConstevalDifferentHeaderSourceTest) {
+  ExtraArgs.push_back("--std=c++20");
+  Header = R"cpp(
+class SomeClass {
+  consteval void f() const;
+};
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+consteval void SomeClass::f() const {
+  [[int x;]]
+}
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+conste