[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 216805.
SureYeaah marked 4 inline comments as done.
SureYeaah added a comment.

Addressed more review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,106 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+  // FIXME: Add tests for macros after selectionTree works properly for macros.
+  // FIXME: This should be extractable
+  EXPECT_EQ(apply("F ([[int x = 0;]])"), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[{}]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct FOO {
+  int x;
+};
+void f(int a) {
+  int b; int *ptr =  FOO foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , int * , struct FOO ) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b; int *ptr =  FOO foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int ) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // 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(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,606 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Extracts statements to a new function and replaces the statements with a
+// call to the new function.
+// Before:
+//   void f(int a) {
+// [[if(a < 5)
+//   a = 5;]]
+//   }
+// After:
+//   void extracted(int ) {
+// if(a < 5)
+//   a = 5;
+//   }
+//   void 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 216669.
SureYeaah marked 33 inline comments as done.
SureYeaah added a comment.
Herald added a subscriber: mgrang.

Addressed Review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,106 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+  // FIXME: Add tests for macros after selectionTree works properly for macros.
+  // FIXME: This should be extractable
+  EXPECT_EQ(apply("F ([[int x = 0;]])"), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[{}]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct FOO {
+  int x;
+};
+void f(int a) {
+  int b; int *ptr =  FOO foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , int * , struct FOO ) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b; int *ptr =  FOO foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int ) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // 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(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,571 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Main themes are:

- there are a few places we can reduce scope for the first patch: e.g. template 
support
- internally, it'd be clearer to pass data around in structs and avoid complex 
classes unless the abstraction is important
- high-level documentation would aid in understanding: for each 
concept/function: what is the goal, how does it fit into higher-level goals, 
are there any non-obvious reasons it's done this way
- naming questions (this can solve some of the same problems as docs, but much 
more cheaply)




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:7
+//
+//===--===//
+#include "ClangdUnit.h"

this file will need an overview describing
 - the refactoring, and (briefly) major user-visible design decisions (e.g. 
params, return types, hoisting)
 - structure/approach/major parts



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:37
+
+// Source is the part of code that is being extracted.
+// EnclosingFunction is the function/method inside which the source lies.

this name is *also* pretty confusing because of `SourceLocation`, `SourceRange` 
etc classes.

Best option might just be to make up something distinctive, like `Zone` :-/



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:39
+// EnclosingFunction is the function/method inside which the source lies.
+// PreSource is everything before source and part of EnclosingFunction.
+// PostSource is everything after source and part of EnclosingFunction.

nit: if you're going to document the values one-by-one, do it in a comment on 
each value instead



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:42
+// We split the file into 4 parts w.r.t. the Source.
+enum LocType { PRESOURCE, INSOURCE, POSTSOURCE, OUTSIDEFUNC };
+

nit: we don't use MACROCASE for enums in LLVM (I think). This of course makes 
namespacing more important, we should consider `enum class` here.

I'd probably suggest
```
enum class ZoneRelative { // or some name consistent with above
  Before,
  After,
  Inside,
  External
};

```



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:45
+// ExtractionSourceView forms a view of the code wrt to Source.
+class ExtractionSourceView {
+public:

This looks like basically a struct (mostly public data members) where all the 
logic is in the constructor.
This would be more obvious/more consistent with style elsewhere if it were 
literally a struct and the constructor was instead a function (which could 
return `Expected` instead of setting flags on the result)



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:45
+// ExtractionSourceView forms a view of the code wrt to Source.
+class ExtractionSourceView {
+public:

sammccall wrote:
> This looks like basically a struct (mostly public data members) where all the 
> logic is in the constructor.
> This would be more obvious/more consistent with style elsewhere if it were 
> literally a struct and the constructor was instead a function (which could 
> return `Expected` instead of setting flags on the 
> result)
nit: not sure `view` adds anything here. `ExtractionSource` or `ExtractionZone`?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:50
+  // Get the location type for a given location.
+  LocType getLocType(SourceLocation Loc) const;
+  // Parent of RootStatements being extracted.

classify?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:59
+  SourceRange EnclosingFunctionRng;
+  const Node *LastRootStmt = nullptr;
+  bool isEligibleForExtraction() const { return IsEligibleForExtraction; }

when is this not Parent->Children.back()?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:93
+  for (const Node *CurNode = CommonAnc; CurNode; CurNode = CurNode->Parent) {
+if (CurNode->ASTNode.get()) {
+  // FIXME: Support extraction from methods.

I think you probably want to avoid extracting from lambdas and templates for 
now, too.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:104
+SourceRange
+ExtractionSourceView::computeEnclosingFunctionRng(const Node 
*EnclosingFunction,
+  const SourceManager ,

nit: Please spell 'range' rather than 'rng' in function names, as it's not a 
standard abbreviation in clang[d]



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:107
+  const LangOptions ) 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:68
+  bool hasOnlyRootStmtChildren();
+  // We only support extraction of RootStmts. A RootStmt as a statement that is
+  // fully selected including all of it's children.

Could you move the definition of `RootStmt` to class definition? Since it is 
used in a lot more places than just this one.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:68
+  bool hasOnlyRootStmtChildren();
+  // We only support extraction of RootStmts. A RootStmt as a statement that is
+  // fully selected including all of it's children.

kadircet wrote:
> Could you move the definition of `RootStmt` to class definition? Since it is 
> used in a lot more places than just this one.
> We only support extraction of RootStmts.

Can you also mention the reasoning/implications ?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:71
+  // Returns the (unselected) parent of all RootStmts.
+  static const Node *getParentOfRootStmts(const Node *CommonAnc);
+  // Find the union of source ranges of all child nodes of Parent. Returns an

Can you make this and the `computeEnclosingFunction{,Rng}` free functions 
instead of static methods?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:74
+  // inavlid SourceRange if it fails to do so.
+  SourceRange computeSourceRng();
+  // Finds the function in which the source lies.

haven't checked the implementation yet, but naming and the comments seem to be 
ambiguous.

Do you mean *selected* childrens of `Parent`, instead of *all*? Because IIUC, 
`Source` refers to the part of the function body that is selected, not all of 
it, right?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:222
+
+public:
+  std::string FuncName = "extracted";

comments for some of the fields like `SemicolonPolicy` would be nice.

also most of these looks like implementation details, consider moving internal 
ones to private



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:286
+
+// Captures information about the source.
+class CapturedSourceInfo {

could you give more details on what is collected and how?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:360
+
+CapturedSourceInfo::DeclInformation &
+CapturedSourceInfo::getDeclInformationFor(const Decl *D) {

why not move this and 4 following functions into astvisitor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526



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

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215615.
SureYeaah added a comment.

Fixed semicolon bug


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,117 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, PrepareFunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+  EXPECT_AVAILABLE(R"cpp(
+[[int sum = 0;
+for(;;)
+  sum++;]]
+for(int i = 0; i < 5; i++) {
+  sum += i;
+}
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp([[int x;]])cpp");
+  // TODO: Add tests for macros after selectionTree works properly for macros.
+  // EXPECT_AVAILABLE(R"cpp( F (int x = 0; [[x = 1;]])cpp");
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  //EXPECT_UNAVAILABLE(R"cpp(int [[x = 0]];)cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+int sum = 0;
+for(;;)
+  [[sum++;
+sum++;]]
+  )cpp");
+  // Expressions aren't extracted.
+  EXPECT_UNAVAILABLE(R"cpp(int x = 0; [[x++;]])cpp");
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  // EXPECT_UNAVAILABLE(R"cpp( for([[int i = 0; i < 5;]] i++) )cpp");
+}
+
+TEST_F(ExtractFunctionTest, PrepareMethodTest) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp");
+}
+
+TEST_F(ExtractFunctionTest, ApplyTest) {
+  // We can extract from templated functions as long as no reference in the
+  // extraction depends on the template.
+  // Checks const qualifier and extraction parameters.
+  Header = R"cpp(
+  )cpp";
+  EXPECT_EQ(apply(
+R"cpp(
+struct FOO {
+  int x;
+};
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  [[a += foo.x;
+  b = c; 
+  *ptr++;
+  for(;;)
+int d = 5 /* check if comment is extracted */ ;]]
+})cpp"),
+R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , const int , struct FOO , int * ) {
+a += foo.x;
+  b = c; 
+  *ptr++;
+  for(;;)
+int d = 5 /* check if comment is extracted */ ;
+}
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  extracted(a, b, c, foo, ptr);
+})cpp");
+  auto ShouldSucceed = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), HasSubstr("extracted"));
+  };
+  auto ShouldFail = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), StartsWith("fail:"));
+  };
+  // Ensure the last break isn't included in the Source since the end of source
+  // and beginning of break are adjacent.
+  ShouldSucceed(R"cpp(
+void f() {
+  for(;;) {
+[[{}]]break;
+  }
+}
+  )cpp");
+  // Don't extract because needs hoisting.
+  ShouldFail(R"cpp( void f() { [[int a = 5;]] a++;})cpp");
+  // Don't extract parameters that depend on template.
+  ShouldFail(R"cpp( template void f() { [[int a = N;]] })cpp");
+  ShouldFail(R"cpp( template void f() { [[T t;]] })cpp");
+  // Don't extract return
+  ShouldFail(R"cpp( int f() { int a = 5; [[return a;]]})cpp");
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this.
+  ShouldFail(R"cpp( int f() { [[for(;;) break;]]})cpp");
+  ShouldFail(R"cpp( int f() { for(;;) [[continue;]]})cpp");
+  // Don't extract when we need to make a function as a parameter.
+  ShouldFail(R"cpp( void f() { [[int a; f();]] } )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,546 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215574.
SureYeaah added a comment.

Fixed bug in getLocType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,115 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, PrepareFunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+  EXPECT_AVAILABLE(R"cpp(
+[[int sum = 0;
+for(;;)
+  sum++;]]
+for(int i = 0; i < 5; i++) {
+  sum += i;
+}
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp([[int x;]])cpp");
+  // TODO: Add tests for macros after selectionTree works properly for macros.
+  // EXPECT_AVAILABLE(R"cpp( F (int x = 0; [[x = 1;]])cpp");
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  //EXPECT_UNAVAILABLE(R"cpp(int [[x = 0]];)cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+int sum = 0;
+for(;;)
+  [[sum++;
+sum++;]]
+  )cpp");
+  // Expressions aren't extracted.
+  EXPECT_UNAVAILABLE(R"cpp(int x = 0; [[x++;]])cpp");
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  // EXPECT_UNAVAILABLE(R"cpp( for([[int i = 0; i < 5;]] i++) )cpp");
+}
+
+TEST_F(ExtractFunctionTest, PrepareMethodTest) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp");
+}
+
+TEST_F(ExtractFunctionTest, ApplyTest) {
+  // We can extract from templated functions as long as no reference in the
+  // extraction depends on the template.
+  // Checks const qualifier and extraction parameters.
+  Header = R"cpp(
+  )cpp";
+  EXPECT_EQ(apply(
+R"cpp(
+struct FOO {
+  int x;
+};
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  [[a += foo.x;
+  b = c; 
+  *ptr++;
+  int d = 5 /* check if comment is extracted */ ;]]
+})cpp"),
+R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , const int , struct FOO , int * ) {
+a += foo.x;
+  b = c; 
+  *ptr++;
+  int d = 5 /* check if comment is extracted */ ;
+}
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  extracted(a, b, c, foo, ptr);
+})cpp");
+  auto ShouldSucceed = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), HasSubstr("extracted"));
+  };
+  auto ShouldFail = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), StartsWith("fail:"));
+  };
+  // Ensure the last break isn't included in the Source since the end of source
+  // and beginning of break are adjacent.
+  ShouldSucceed(R"cpp(
+void f() {
+  for(;;) {
+[[{}]]break;
+  }
+}
+  )cpp");
+  // Don't extract because needs hoisting.
+  ShouldFail(R"cpp( void f() { [[int a = 5;]] a++;})cpp");
+  // Don't extract parameters that depend on template.
+  ShouldFail(R"cpp( template void f() { [[int a = N;]] })cpp");
+  ShouldFail(R"cpp( template void f() { [[T t;]] })cpp");
+  // Don't extract return
+  ShouldFail(R"cpp( int f() { int a = 5; [[return a;]]})cpp");
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this.
+  ShouldFail(R"cpp( int f() { [[for(;;) break;]]})cpp");
+  ShouldFail(R"cpp( int f() { for(;;) [[continue;]]})cpp");
+  // Don't extract when we need to make a function as a parameter.
+  ShouldFail(R"cpp( void f() { [[int a; f();]] } )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,537 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215131.
SureYeaah added a comment.

Better Naming


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,103 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, PrepareFunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+  EXPECT_AVAILABLE(R"cpp(
+[[int sum = 0;
+for(;;)
+  sum++;]]
+for(int i = 0; i < 5; i++) {
+  sum += i;
+}
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp([[int x;]])cpp");
+  // TODO: Add tests for macros after selectionTree works properly for macros.
+  // EXPECT_AVAILABLE(R"cpp( F (int x = 0; [[x = 1;]])cpp");
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  //EXPECT_UNAVAILABLE(R"cpp(int [[x = 0]];)cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+int sum = 0;
+for(;;)
+  [[sum++;
+sum++;]]
+  )cpp");
+  // Expressions aren't extracted.
+  EXPECT_UNAVAILABLE(R"cpp(int x = 0; [[x++;]])cpp");
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  // EXPECT_UNAVAILABLE(R"cpp( for([[int i = 0; i < 5;]] i++) )cpp");
+}
+
+TEST_F(ExtractFunctionTest, PrepareMethodTest) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp");
+}
+
+TEST_F(ExtractFunctionTest, ApplyTest) {
+  // We can extract from templated functions as long as no reference in the
+  // extraction depends on the template.
+  // Checks const qualifier and extraction parameters.
+  Header = R"cpp(
+  )cpp";
+  EXPECT_EQ(apply(
+R"cpp(
+struct FOO {
+  int x;
+};
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  [[a += foo.x;
+  b = c; 
+  *ptr++;
+  int d = 5 /* check if comment is extracted */ ;]]
+})cpp"),
+R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , const int , struct FOO , int * ) {
+a += foo.x;
+  b = c; 
+  *ptr++;
+  int d = 5 /* check if comment is extracted */ ;
+}
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  extracted(a, b, c, foo, ptr);
+})cpp");
+  auto ShouldFail = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), StartsWith("fail:"));
+  };
+  // Don't extract because needs hoisting.
+  ShouldFail(R"cpp( void f() { [[int a = 5;]] a++;})cpp");
+  // Don't extract parameters that depend on template.
+  ShouldFail(R"cpp( template void f() { [[int a = N;]] })cpp");
+  ShouldFail(R"cpp( template void f() { [[T t;]] })cpp");
+  // Don't extract return
+  ShouldFail(R"cpp( int f() { int a = 5; [[return a;]]})cpp");
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this.
+  ShouldFail(R"cpp( int f() { [[for(;;) break;]]})cpp");
+  ShouldFail(R"cpp( int f() { for(;;) [[continue;]]})cpp");
+  // Don't extract when we need to make a function as a parameter.
+  ShouldFail(R"cpp( void f() { [[int a; f();]] } )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,534 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215098.
SureYeaah added a comment.

Removed debug info


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,97 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, PrepareFunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+  EXPECT_AVAILABLE(R"cpp(
+[[int sum = 0;
+for(;;)
+  sum++;]]
+for(int i = 0; i < 5; i++) {
+  sum += i;
+}
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp([[int x;]])cpp");
+  // TODO: Add tests for macros after selectionTree works properly for macros.
+  // EXPECT_AVAILABLE(R"cpp( F (int x = 0; [[x = 1;]])cpp");
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  //EXPECT_UNAVAILABLE(R"cpp(int [[x = 0]];)cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+int sum = 0;
+for(;;)
+  [[sum++;
+sum++;]]
+  )cpp");
+  // Expressions aren't extracted.
+  EXPECT_UNAVAILABLE(R"cpp(int x = 0; [[x++;]])cpp");
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  // EXPECT_UNAVAILABLE(R"cpp( for([[int i = 0; i < 5;]] i++) )cpp");
+}
+
+TEST_F(ExtractFunctionTest, PrepareMethodTest) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp");
+}
+
+TEST_F(ExtractFunctionTest, ApplyTest) {
+  // We can extract from templated functions as long as no reference in the
+  // extraction depends on the template.
+  // Checks const qualifier and extraction parameters.
+  Header = R"cpp(
+  )cpp";
+  EXPECT_EQ(apply(
+R"cpp(
+struct FOO {
+  int x;
+};
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  [[a += foo.x; b = c; *ptr++;]]
+})cpp"),
+R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , const int , struct FOO , int * ) {
+a += foo.x; b = c; *ptr++;
+}
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  int *ptr = 
+  extracted(a, b, c, foo, ptr);
+})cpp");
+  auto ShouldFail = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), StartsWith("fail:"));
+  };
+  // Don't extract because needs hoisting.
+  ShouldFail(R"cpp( void f() { [[int a = 5;]] a++;})cpp");
+  // Don't extract parameters that depend on template.
+  ShouldFail(R"cpp( template void f() { [[int a = N;]] })cpp");
+  ShouldFail(R"cpp( template void f() { [[T t;]] })cpp");
+  // Don't extract return
+  ShouldFail(R"cpp( int f() { int a = 5; [[return a;]]})cpp");
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this.
+  ShouldFail(R"cpp( int f() { [[for(;;) break;]]})cpp");
+  ShouldFail(R"cpp( int f() { for(;;) [[continue;]]})cpp");
+  // Don't extract when we need to make a function as a parameter.
+  ShouldFail(R"cpp( void f() { [[int a; f();]] } )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,534 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using Node = 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215089.
SureYeaah added a comment.

Removed unrelated changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.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
@@ -599,6 +599,93 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, PrepareFunctionTest) {
+  Header = R"cpp(
+#define F(BODY) void FFF() { BODY }
+  )cpp";
+  Context = Function;
+  EXPECT_AVAILABLE(R"cpp(
+[[int sum = 0;
+for(;;)
+  sum++;]]
+for(int i = 0; i < 5; i++) {
+  sum += i;
+}
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp([[int x;]])cpp");
+  // TODO: Add tests for macros after selectionTree works properly for macros.
+  // EXPECT_AVAILABLE(R"cpp( F (int x = 0; [[x = 1;]])cpp");
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  //EXPECT_UNAVAILABLE(R"cpp(int [[x = 0]];)cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+int sum = 0;
+for(;;)
+  [[sum++;
+sum++;]]
+  )cpp");
+  // Expressions aren't extracted.
+  EXPECT_UNAVAILABLE(R"cpp(int x = 0; [[x++;]])cpp");
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  // EXPECT_UNAVAILABLE(R"cpp( for([[int i = 0; i < 5;]] i++) )cpp");
+}
+
+TEST_F(ExtractFunctionTest, PrepareMethodTest) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp");
+}
+
+TEST_F(ExtractFunctionTest, ApplyTest) {
+  // We can extract from templated functions as long as no reference in the
+  // extraction depends on the template.
+  // Checks const qualifier and extraction parameters.
+  Header = R"cpp(
+  )cpp";
+  EXPECT_EQ(apply(
+R"cpp(
+struct FOO {
+  int x;
+};
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  [[a += foo.x; b = c;]]
+})cpp"),
+R"cpp(
+struct FOO {
+  int x;
+};
+void extracted(int , int , const int , struct FOO ) {a += foo.x; b = c;}
+template
+void f(int a) {
+  int b = N; T t; const int c; FOO foo;
+  extracted(a, b, c, foo);
+})cpp");
+  auto ShouldFail = [&](llvm::StringRef Code) {
+EXPECT_THAT(apply(Code), StartsWith("fail:"));
+  };
+  // Don't extract because needs hoisting.
+  ShouldFail(R"cpp( void f() { [[int a = 5;]] a++;})cpp");
+  // Don't extract parameters that depend on template.
+  ShouldFail(R"cpp( template void f() { [[int a = N;]] })cpp");
+  ShouldFail(R"cpp( template void f() { [[T t;]] })cpp");
+  // Don't extract return
+  ShouldFail(R"cpp( int f() { int a = 5; [[return a;]]})cpp");
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this.
+  ShouldFail(R"cpp( int f() { [[for(;;) break;]]})cpp");
+  ShouldFail(R"cpp( int f() { for(;;) [[continue;]]})cpp");
+  // Don't extract when we need to make a function as a parameter.
+  ShouldFail(R"cpp( void f() { [[int a; f();]] } )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,536 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using Node = SelectionTree::Node;
+
+// Source is the part of code that is 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 215077.
SureYeaah marked 11 inline comments as done.
SureYeaah added a comment.

Refactored design and added unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/TweakTesting.h
  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
@@ -133,7 +133,8 @@
   EXPECT_EQ(apply("^if (true) {return 100;} else {continue;}"),
 "if (true) {continue;} else {return 100;}");
   EXPECT_EQ(apply("^if () {return 100;} else {continue;}"),
-"if () {continue;} else {return 100;}") << "broken condition";
+"if () {continue;} else {return 100;}")
+  << "broken condition";
   EXPECT_AVAILABLE("^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }");
   EXPECT_UNAVAILABLE("if (true) {^return ^100;^ } else { ^continue^;^ }");
   // Available in subexpressions of the condition;
@@ -164,7 +165,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -345,11 +346,11 @@
  void f(int a) {
int y = PLUS([[1+a]]);
  })cpp",
-  /*FIXME: It should be extracted like this.
-   R"cpp(#define PLUS(x) x++
- void f(int a) {
-   auto dummy = 1+a; int y = PLUS(dummy);
- })cpp"},*/
+   /*FIXME: It should be extracted like this.
+R"cpp(#define PLUS(x) x++
+  void f(int a) {
+auto dummy = 1+a; int y = PLUS(dummy);
+  })cpp"},*/
R"cpp(#define PLUS(x) x++
  void f(int a) {
auto dummy = PLUS(1+a); int y = dummy;
@@ -360,13 +361,13 @@
if(1)
 LOOP(5 + [[3]])
  })cpp",
-  /*FIXME: It should be extracted like this. SelectionTree needs to be
-* fixed for macros.
-   R"cpp(#define LOOP(x) while (1) {a = x;}
-   void f(int a) {
- auto dummy = 3; if(1)
-  LOOP(5 + dummy)
-   })cpp"},*/
+   /*FIXME: It should be extracted like this. SelectionTree needs to be
+ * fixed for macros.
+R"cpp(#define LOOP(x) while (1) {a = x;}
+void f(int a) {
+  auto dummy = 3; if(1)
+   LOOP(5 + dummy)
+})cpp"},*/
R"cpp(#define LOOP(x) while (1) {a = x;}
  void f(int a) {
auto dummy = LOOP(5 + 3); if(1)
@@ -462,8 +463,8 @@
  void f() {
auto dummy = S(2) + S(3) + S(4); S x = S(1) + dummy + S(5);
  })cpp"},
-   // Don't try to analyze across macro boundaries
-   // FIXME: it'd be nice to do this someday (in a safe way)
+  // Don't try to analyze across macro boundaries
+  // FIXME: it'd be nice to do this someday (in a safe way)
   {R"cpp(#define ECHO(X) X
  void f() {
int x = 1 + [[ECHO(2 + 3) + 4]] + 5;
@@ -491,26 +492,27 @@
   checkAvailable(ID, "^vo^id^ ^f(^) {^}^"); // available everywhere.
   checkAvailable(ID, "[[int a; int b;]]");
   const char *Input = "void ^f() {}";
-  const char *Output = "/* storage.type.primitive.cpp */void /* entity.name.function.cpp */f() {}";
+  const char *Output = "/* storage.type.primitive.cpp */void /* "
+   "entity.name.function.cpp */f() {}";
   checkTransform(ID, Input, Output);
 
   checkTransform(ID,
-  R"cpp(
+ R"cpp(
 [[void f1();
 void f2();]]
 )cpp",
-  R"cpp(
+ R"cpp(
 /* storage.type.primitive.cpp */void /* entity.name.function.cpp */f1();
 /* storage.type.primitive.cpp */void /* entity.name.function.cpp */f2();
 )cpp");
 
-   checkTransform(ID,
-  R"cpp(
+  checkTransform(ID,
+ R"cpp(
 void f1();
 void f2() {^};
 )cpp",
 
-  R"cpp(
+ R"cpp(
 void f1();
 /* storage.type.primitive.cpp */void /* entity.name.function.cpp */f2() {};
 )cpp");
@@ -590,7 +592,7 @@
   StartsWith("fail: Could not expand type of lambda expression"));
   // inline 

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:211
+  // Note that DRELocType is never OUTSIDECONTEXT.
+  if (DeclLocType + 1 != DRELocType)
+return;

SureYeaah wrote:
> sammccall wrote:
> > this line seems very suspect, what are you trying to do and why?
> > 
> This just checks whether (DeclLocType, DRELocType) is either (PRETARGET, 
> TARGET) or (TARGET, POSTTARGET). Could explicitly write it if needed.
Definitely needed, apart from the lack of clarity the current version has UB 
sometimes (`DeclLocType + 1` might not be a valid value)

But by "why" i mostly mean - this looks like the place for recording the 
location, not the place for deciding not to record the location because some 
algorithm later may not need it. Seems a lot simpler to record it 
unconditionally and worry about it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526



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

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah marked 2 inline comments as done.
SureYeaah added a comment.

In D65526#1612117 , @sammccall wrote:

> I guess you're looking for design review at this point.
>
> But the most important part of that is the documentation of the high-level 
> structure, which is entirely missing. I can't really infer it from the code 
> because most of the design is (presumably) not implemented at this point :-)


Aaah. Yes. Sorry. Makes sense.




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:110
+  // FIXME: Bail out when it's partial selected. Wait for selectiontree
+  // semicolon fix.
+  if (CommonAnc->Selected == SelectionTree::Selection::Partial &&

sammccall wrote:
> what is the fix you're waiting for?
The semicolon fix that has already landed now.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:211
+  // Note that DRELocType is never OUTSIDECONTEXT.
+  if (DeclLocType + 1 != DRELocType)
+return;

sammccall wrote:
> this line seems very suspect, what are you trying to do and why?
> 
This just checks whether (DeclLocType, DRELocType) is either (PRETARGET, 
TARGET) or (TARGET, POSTTARGET). Could explicitly write it if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526



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

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I guess you're looking for design review at this point.

But the most important part of that is the documentation of the high-level 
structure, which is entirely missing. I can't really infer it from the code 
because most of the design is (presumably) not implemented at this point :-)




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:36
+
+class ExtractionTarget {
+public:

I think "range" or "region" may be a clearer name here than "target".
(In extract variable, target was used to refer to the destination of extraction)



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:110
+  // FIXME: Bail out when it's partial selected. Wait for selectiontree
+  // semicolon fix.
+  if (CommonAnc->Selected == SelectionTree::Selection::Partial &&

what is the fix you're waiting for?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:155
+public:
+  enum LocType { PRETARGET, TARGET, POSTTARGET, OUTSIDECONTEXT };
+  struct Reference {

These are important domain structures, can we move them out of the 
RecursiveASTVisitor? (which should be mostly restricted to managing state of 
the traversal itself)



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:156
+  enum LocType { PRETARGET, TARGET, POSTTARGET, OUTSIDECONTEXT };
+  struct Reference {
+Decl *TheDecl = nullptr;

You've called this "reference", but I think it's actually a "referenceddecl" - 
you don't store one of these for each reference do you?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:189
+
+TargetAnalyzer::Reference ::getReferenceFor(VarDecl *D) {
+  assert(D && "D shouldn't be null!");

why does D need to be a vardecl?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:190
+TargetAnalyzer::Reference ::getReferenceFor(VarDecl *D) {
+  assert(D && "D shouldn't be null!");
+  // Don't add Decls that are outside the extraction context or in PostTarget.

please don't add messages to asserts that just echo the code. In order of 
preference:
 - take a reference so it clearly can't be null
 - add a message explaining at a *high level* what variant is violated
 - add the assertion with no message



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:197
+
+void TargetAnalyzer::checkIfRecursiveCall(DeclRefExpr *DRE) {
+  if (Target->isInTarget(DRE->getBeginLoc()) &&

why is this handled specially, instead of being part of the general case of 
"referring to a decl not visible in the target scope"?

(in particular, if the context is forward-declared elsewhere, what's the 
problem?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:203
+
+void TargetAnalyzer::updateReferenceFor(DeclRefExpr *DRE) {
+  VarDecl *D = dyn_cast_or_null(DRE->getDecl());

DRE is just one of the cases where we want to handle names.

I think it'll be easier to handle more cases by changing this to accept a Decl* 
and moving the DRE->getDecl() to the caller



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:207
+return;
+  LocType DRELocType = getLocType(DRE->getBeginLoc());
+  LocType DeclLocType = getLocType(D->getBeginLoc());

(why beginloc rather than loc, throughout?)



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:211
+  // Note that DRELocType is never OUTSIDECONTEXT.
+  if (DeclLocType + 1 != DRELocType)
+return;

this line seems very suspect, what are you trying to do and why?




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:214
+  Reference  = getReferenceFor(D);
+  if (DRELocType == POSTTARGET)
+Ref.IsReferencedInPostTarget = true;

can't we skip the indirection here and just have something like 
Ref.markOccurrence(DRE->getBeginLoc()), and avoid dealing with the enum here at 
all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526



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

[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah created this revision.
SureYeaah added reviewers: kadircet, sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, 
ilya-biryukov, mgorny.
Herald added a project: clang.

- Prototype version; Only looking for a high level review.
- Only works for extraction from free functions
- Basic analysis of the code being extracted.
- Extracts to a void function with no parameters.
- Doesn't handle semicolons yet.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,285 @@
+//===--- ExtractFunction.cpp *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using Node = SelectionTree::Node;
+using std::shared_ptr;
+
+class ExtractionTarget {
+public:
+  ExtractionTarget(const Node *N, const SourceManager ,
+   const ASTContext );
+
+  // Check if the location is part of the TargetContext and before Target.
+  bool isInPreTarget(SourceLocation Loc) const {
+return !isOutsideTargetContext(Loc) && Loc < TargetRng.getBegin();
+  }
+
+  // Checks whether the Location is within the Target.
+  bool isInTarget(SourceLocation Loc) const {
+return SM.isPointWithin(Loc, TargetRng.getBegin(), TargetRng.getEnd());
+  }
+
+  // Check if the location is part of the TargetContext and after Target.
+  bool isInPostTarget(SourceLocation Loc) const {
+return !isOutsideTargetContext(Loc) && TargetRng.getEnd() < Loc;
+  }
+  // Check if the location is outside the TargetContext.
+  bool isOutsideTargetContext(SourceLocation Loc) const {
+return !SM.isPointWithin(Loc, TargetContextRng.getBegin(),
+ TargetContextRng.getEnd());
+  }
+  bool isExtractable() { return TargetRng.isValid() && TargetContext; }
+
+  tooling::Replacement replaceWithFuncCall(llvm::StringRef FuncName) const;
+  tooling::Replacement createFunctionDefinition(llvm::StringRef FuncName) const;
+  // The function inside which our target resides.
+  const FunctionDecl *TargetContext;
+
+private:
+  const Node *CommonAnc;
+  const SourceManager 
+  const ASTContext 
+  // The range of the code being extracted.
+  SourceRange TargetRng;
+  SourceRange TargetContextRng;
+
+  SourceRange computeTargetRange() const;
+};
+
+const FunctionDecl *computeTargetContext(const Node *CommonAnc) {
+  // Walk up the SelectionTree until we find a function Decl
+  for (const Node *CurNode = CommonAnc; CurNode; CurNode = CurNode->Parent) {
+if (const FunctionDecl *FD = CurNode->ASTNode.get()) {
+  // FIXME: Support extraction from methods.
+  if (CurNode->ASTNode.get())
+return nullptr;
+  return FD;
+}
+  }
+  return nullptr;
+}
+
+ExtractionTarget::ExtractionTarget(const Node *CommonAnc,
+   const SourceManager ,
+   const ASTContext )
+: CommonAnc(CommonAnc), SM(SM), Ctx(Ctx) {
+  TargetRng = computeTargetRange();
+  if ((TargetContext = computeTargetContext(CommonAnc)))
+TargetContextRng =
+TargetContext->getSourceRange(); // TODO: Use toHalfOpenFileRange?
+}
+
+// TODO: Describe how this works
+SourceRange ExtractionTarget::computeTargetRange() const {
+  const LangOptions  = Ctx.getLangOpts();
+  if (!CommonAnc)
+return SourceRange();
+  if (CommonAnc->Selected == SelectionTree::Selection::Complete)
+return *toHalfOpenFileRange(SM, LangOpts,
+CommonAnc->ASTNode.getSourceRange());
+  // FIXME: Bail out when it's partial selected. Wait for selectiontree
+  // semicolon fix.
+  if (CommonAnc->Selected == SelectionTree::Selection::Partial &&
+  !CommonAnc->ASTNode.get())
+return