[PATCH] D65526: [Clangd] First version of ExtractFunction
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370249: [Clangd] Initial version of ExtractFunction
(authored by SureYeaah, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D65526?vs=217701&id=217702#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65526/new/
https://reviews.llvm.org/D65526
Files:
clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
@@ -500,6 +500,112 @@
R"cpp(const char * x = "test")cpp");
}
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+ 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");
+
+ // 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(;;) { [[int x;]]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 = &a;
+ Foo foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b;
+ int *ptr = &a;
+ 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 &c) {
+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");
+
+ // FIXME: This should be extractable after selectionTree works correctly for
+ // macros (currently it doesn't select anything for the following case)
+ std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+ )cpp";
+ EXPECT_EQ(apply(MacroFailInput), "unavailable");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,605 @@
+//===--- 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-exce
[PATCH] D65526: [Clangd] First version of ExtractFunction
SureYeaah added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:119 + case SelectionTree::Selection::Partial: +// Treat Partially selected VarDecl as completely selected since +// SelectionTree doesn't always select VarDecls correctly. sammccall wrote: > D66872 has the fix if you'd rather avoid these workarounds Will remove this once D66872 is committed. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150 + } + bool isRootStmt(const Stmt *S) const; + // The last root statement is important to decide where we need to insert a kadircet wrote: > it seems like you are rather using it to decide whether a statement is inside > the zone or not? > > Could you rather rename it to reflect that and add some comments? if extracting ``` for(;;) int x = 0; ``` the DeclStmt is inside the zone but not a rootstmt. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172 + // FIXME: Support extraction from templated functions. + if (CurNode->Parent->ASTNode.get()) +return nullptr; kadircet wrote: > nit: > ``` > if(isa(Func)) > ``` Using `Func->isTemplated()` Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346 +ZoneRelative DeclaredIn; +// index of the declaration or first reference +unsigned DeclIndex; kadircet wrote: > i think you mean `index of the first reference to this decl` ? It can be the index of declaration or the first reference. Since the visitor Visits Decls as well, we will encounter a reference before the Decl only if the Decl is outside the enclosing function. 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] First version of ExtractFunction
SureYeaah updated this revision to Diff 217701.
SureYeaah marked 14 inline comments as done.
SureYeaah added a comment.
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,112 @@
R"cpp(const char * x = "test")cpp");
}
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+ 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");
+
+ // 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(;;) { [[int x;]]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 = &a;
+ Foo foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b;
+ int *ptr = &a;
+ 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 &c) {
+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");
+
+ // FIXME: This should be extractable after selectionTree works correctly for
+ // macros (currently it doesn't select anything for the following case)
+ std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+ )cpp";
+ EXPECT_EQ(apply(MacroFailInput), "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,604 @@
+//===--- 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;]]
+//
[PATCH] D65526: [Clangd] First version of ExtractFunction
kadircet marked an inline comment as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+else
+ SR.setEnd(ChildFileRange->getEnd());
+ }
SureYeaah wrote:
> kadircet wrote:
> > I suppose this relies on the fact that "AST contains the nodes ordered by
> > their begin location"? Could you add an assertion for that?
> I've removed the loop, should I still add this?
no need
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
SureYeaah wrote:
> SureYeaah wrote:
> > sammccall wrote:
> > > kadircet wrote:
> > > > why is this function returning a shared_ptr ?
> > > avoid shared_ptr unless there's a strong reason.
> > > `Optional` seems fine here?
> > And store a unique_ptr to the optional in ExtractFunction?
> Because ExtractFunction needs to store a pointer/reference to ExtractionZone
> somehow in prepare and access it in apply.
Let's figure it out in the `ExtractFunction` this can still return an
`Optional` which you can convert into a pointer/reference later
on if need be?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+ std::shared_ptr ExtZone;
+};
SureYeaah wrote:
> kadircet wrote:
> > why do you need a shared_ptr here?
> getExtractionZone creates an Optional which needs to persist
> from prepare to apply. Is there a better way to store a reference to the
> ExtractionZone instance inside ExtractFunction?
Then why not just store `Optional` ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+ SourceRange EnclosingFuncRange;
+ std::set RootStmts;
+ SourceLocation getInsertionPoint() const {
lifetime of this field looks complicated can you add some comments on how/when
it is initialized and maybe enforce immutability ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150
+ }
+ bool isRootStmt(const Stmt *S) const;
+ // The last root statement is important to decide where we need to insert a
it seems like you are rather using it to decide whether a statement is inside
the zone or not?
Could you rather rename it to reflect that and add some comments?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:169
+ // FIXME: Support extraction from methods.
+ if (CurNode->ASTNode.get())
+return nullptr;
nit:
```
if (isa(Func))
```
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172
+ // FIXME: Support extraction from templated functions.
+ if (CurNode->Parent->ASTNode.get())
+return nullptr;
nit:
```
if(isa(Func))
```
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+ZoneRelative DeclaredIn;
+// index of the declaration or first reference
+unsigned DeclIndex;
i think you mean `index of the first reference to this decl` ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:486
+// (this includes the case of recursive call to EnclosingFunc in Zone).
+if (!VD || dyn_cast_or_null(DeclInfo.TheDecl))
+ return false;
nit `isa` instead of `dyn_cast`
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] First version of ExtractFunction
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
I think we should go ahead and land this. The only points that I'd really like
to see fixed is `shared_ptr`, mostly because it's a strong signal there's
something complicated going on and I don't think (or hope) there is!
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+ return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};
sammccall wrote:
> use `printType` from AST.h?
>
> (You'll want to drop qualifiers/refs before calling that, but it's not at all
> obvious from the function name here that they're dropped, so that should be
> at the callsite anyway)
Second half is not done: the suggestion is that it's really confusing where
spellType is called that it drops qualifiers, so just I'd inline this into the
callsite.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+ if (WithTypeAndQualifiers) {
+if (IsConst)
+ Result += "const ";
SureYeaah wrote:
> kadircet wrote:
> > why don't we infer this const from QualType ?
> In a future patch we will want to use heuristics like has the variable been
> assigned, was it passed as a non-const reference, was its address taken, etc.
I think the point is that the QualType in parameter could/should represent the
*parameter* type, not the type of the variable being captured.
e.g. it could be `const std::string&` even if the original variable was just
`std::string`.
This seems the more natural model (the struct is called Parameter, not
CapturedVariable or Argument) but there may be reasons not to do this.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+ std::shared_ptr ExtZone;
+};
SureYeaah wrote:
> kadircet wrote:
> > why do you need a shared_ptr here?
> getExtractionZone creates an Optional which needs to persist
> from prepare to apply. Is there a better way to store a reference to the
> ExtractionZone instance inside ExtractFunction?
shared ownership is less common and harder to reason about than exclusive
ownership
Generally we prefer deciding where ownership should reside (T or Optional or
unique_ptr), and where you should have an unowned reference (T& or T*).
In this case, findExtractionZone() should ideally return
`Optional`, after prepare() the ExtractFunction class should
own it (as a Optional), and apply should pass it around as a
const ExtractionZone&.
If it turns out ExtractionZone isn't a movable type, then
`Optional` won't work and you'll need
`unique_ptr` instead. But shared_ptr shouldn't be needed
regardless.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:119
+ case SelectionTree::Selection::Partial:
+// Treat Partially selected VarDecl as completely selected since
+// SelectionTree doesn't always select VarDecls correctly.
D66872 has the fix if you'd rather avoid these workarounds
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+ SourceRange EnclosingFuncRange;
+ std::set RootStmts;
+ SourceLocation getInsertionPoint() const {
llvm::DenseSet
std::set is a pretty terrible data structure unless you really really need
order.
(Lots of allocations, lots of pointer-chasing)
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:238
+ // Convert RootStmt Nodes to Stmts and insert in RootStmts.
+ llvm::transform(ExtZone->Parent->Children,
+ std::inserter(ExtZone->RootStmts,
ExtZone->RootStmts.begin()),
this is also:
```
for (const SelectionTree::Node *N)
ExtZone->RootStmts.insert(N->ASTNode.get());
```
which is shorter and (I think) less obfuscated
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] First version of ExtractFunction
SureYeaah added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+else
+ SR.setEnd(ChildFileRange->getEnd());
+ }
kadircet wrote:
> I suppose this relies on the fact that "AST contains the nodes ordered by
> their begin location"? Could you add an assertion for that?
I've removed the loop, should I still add this?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+ for (const Node *Child : Parent->Children) {
sammccall wrote:
> kadircet wrote:
> > `hasOnlyRootStmtsAsChildren` ?
> nit: I think this would be clearer as
> `canBeRootStmt(const Node*)` and write the callsite as
> `llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though
Replaced with isRootStmt
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
sammccall wrote:
> kadircet wrote:
> > why is this function returning a shared_ptr ?
> avoid shared_ptr unless there's a strong reason. `Optional`
> seems fine here?
And store a unique_ptr to the optional in ExtractFunction?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
SureYeaah wrote:
> sammccall wrote:
> > kadircet wrote:
> > > why is this function returning a shared_ptr ?
> > avoid shared_ptr unless there's a strong reason. `Optional`
> > seems fine here?
> And store a unique_ptr to the optional in ExtractFunction?
Because ExtractFunction needs to store a pointer/reference to ExtractionZone
somehow in prepare and access it in apply.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+ NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy,
sammccall wrote:
> this is just initializing public fields, drop the constructor?
> (The callsite is clearer here if you initialize them by name)
`ExtractionSemicolonPolicy` doesn't have a default constructor.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+ if (WithTypeAndQualifiers) {
+if (IsConst)
+ Result += "const ";
kadircet wrote:
> why don't we infer this const from QualType ?
In a future patch we will want to use heuristics like has the variable been
assigned, was it passed as a non-const reference, was its address taken, etc.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+ std::shared_ptr ExtZone;
+};
kadircet wrote:
> why do you need a shared_ptr here?
getExtractionZone creates an Optional which needs to persist
from prepare to apply. Is there a better way to store a reference to the
ExtractionZone instance inside ExtractFunction?
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:611
+ EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+ // Expressions aren't extracted.
+ EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
sammccall wrote:
> wait, what?
> expressions that *aren't statements* shouldn't be extracted.
> But what's wrong with this one?
> (Many useful statements are expressions, such as function calls)
For now we don't extract if the selection is an expression. I've added a fixme
to change that to sub-expressions only.
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] First version of ExtractFunction
SureYeaah updated this revision to Diff 217620.
SureYeaah added a comment.
NFC: Whitespace formatting
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,112 @@
R"cpp(const char * x = "test")cpp");
}
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+ 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");
+
+ // 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(;;) { [[int x;]]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 = &a;
+ Foo foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b;
+ int *ptr = &a;
+ 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 &c) {
+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");
+
+ // FIXME: This should be extractable after selectionTree works correctly for
+ // macros (currently it doesn't select anything for the following case)
+ std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+ )cpp";
+ EXPECT_EQ(apply(MacroFailInput), "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,608 @@
+//===--- 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 &a) {
[PATCH] D65526: [Clangd] First version of ExtractFunction
SureYeaah updated this revision to Diff 217619.
SureYeaah marked 45 inline comments as done.
SureYeaah added a comment.
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,112 @@
R"cpp(const char * x = "test")cpp");
}
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+ 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");
+
+ // 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(;;) { [[int x;]]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 = &a;
+ Foo foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b;
+ int *ptr = &a;
+ 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 &c) {
+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");
+
+ // FIXME: This should be extractable after selectionTree works correctly for
+ // macros (currently it doesn't select anything for the following case)
+ std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+ )cpp";
+ EXPECT_EQ(apply(MacroFailInput), "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,608 @@
+//===--- 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;]]
+//
[PATCH] D65526: [Clangd] First version of ExtractFunction
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:46
+//creating a NewFunction.
+// Design Doc at
+//===--===//
please fix or remove comment
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:88
+
+// The ExtractionZone class forms a view of the code wrt to Zone.
+// A RootStmt is a statement that's fully selected including all it's children
nit: "wrt to" -> wrt
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:104
+ SourceRange EnclosingFuncRange;
+ const SourceManager &SM;
+
remove from struct and pass as a parameter?
This isn't really data
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:112
+ }
+ const Node *getLastRootStmt() const {
+return Parent ? Parent->Children.back() : nullptr;
document why this is important, or just inline it?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:113
+ const Node *getLastRootStmt() const {
+return Parent ? Parent->Children.back() : nullptr;
+ }
don't bother handling the null parent case. When can that happen? We should
avoid creating an ExtractionZone struct entirely then.
Also none of the callsites bother to check for null...
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:132
+}
+// Finds the function in which the zone lies.
+const Node *computeEnclosingFunction(const Node *CommonAnc) {
nit: consistently one blank line between functions (in this file you alternate
between 1/0)
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:157
+ SourceRange SR;
+ for (const Node *Child : Parent->Children) {
+auto ChildFileRange =
why are you doing this with a loop, isn't it just {first->begin,last->end}?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+ for (const Node *Child : Parent->Children) {
kadircet wrote:
> `hasOnlyRootStmtsAsChildren` ?
nit: I think this would be clearer as
`canBeRootStmt(const Node*)` and write the callsite as
`llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
kadircet wrote:
> why is this function returning a shared_ptr ?
avoid shared_ptr unless there's a strong reason. `Optional`
seems fine here?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:257
+unsigned OrderPriority; // Lower value parameters are preferred first.
+std::string render(bool WithTypeAndQualifiers) const;
+bool operator<(const Parameter &Other) const {
there's no need for the WithTypeAndQualifiers=false version, that's just
"fn.name"
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+ NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy,
this is just initializing public fields, drop the constructor?
(The callsite is clearer here if you initialize them by name)
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:280
+ // Add a new parameter for the function.
+ void addParam(llvm::StringRef Name, QualType TypeInfo, bool IsConst,
+bool IsReference, unsigned OrderPriority);
this is just push_back, inline into caller
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:285
+ const SourceManager &SM;
+ std::string renderParameters(bool WithTypeAndQualifiers) const;
+ // Generate the function body.
again, avoid sharing code between the complicated declaration case, and the
trivial callsite case. It makes changes harder and obscures the hard case.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+ return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};
use `printType` from AST.h?
(You'll want to drop qualifiers/refs before calling that, but it's not at all
obvious from the function name here that they're dropped, so that should be at
the callsite anyway)
===
[PATCH] D65526: [Clangd] First version of ExtractFunction
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:29
+// - Always passed by l-value reference
+// - No return
+// - Cannot move declarations before extracting
did you mean no return *type* ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:30
+// - No return
+// - Cannot move declarations before extracting
+// - Doesn't check for broken control flow
don't understand what you meant here
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:31
+// - Cannot move declarations before extracting
+// - Doesn't check for broken control flow
+//
could you elaborate on that one?
You can ignore this comment if it is gonna be explained in the design doc(and
you are planning to attach it before landing this revision)
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:42
+//methods for rendering it.
+// 4. CapturedZoneInfo uses a RAV to capture information about the extraction
+//like declarations, existing return statements, broken control flow, etc.
Tried hard to figure out what RAV is :D could you rather say
`RecursiveASTVisitor(RAV)` ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:122
+return ZoneRelative::OutsideFunc;
+ if (Loc < ZoneRange.getBegin())
+return ZoneRelative::Before;
IIRC, `<` operator in `SourceLocation`s are merely for implementing hashing and
doesn't really carry `less than` semantics. Could you rather use
`SM.isBeforeInSLocAddrSpace`?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:124
+return ZoneRelative::Before;
+ if (Loc < ZoneRange.getEnd())
+return ZoneRelative::Inside;
same here
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:133
+// Finds the function in which the zone lies.
+const Node *computeEnclosingFunction(const Node *CommonAnc) {
+ // Walk up the SelectionTree until we find a function Decl
Can you rather return a `FunctionDecl` ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:152
+
+// Find the union of source ranges of all child nodes of Parent. Returns an
+// invalid SourceRange if it fails to do so.
maybe rather return an llvm::Optional/Expected ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+else
+ SR.setEnd(ChildFileRange->getEnd());
+ }
I suppose this relies on the fact that "AST contains the nodes ordered by their
begin location"? Could you add an assertion for that?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:176
+ const LangOptions &LangOpts) {
+ return *toHalfOpenFileRange(SM, LangOpts,
+ EnclosingFunction->ASTNode.getSourceRange());
what makes this fail-free ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:180
+
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
This explains *what* the function is doing, but lacks the *reason why*
Could you explain the reason either in here or in the call site?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+ for (const Node *Child : Parent->Children) {
`hasOnlyRootStmtsAsChildren` ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:215
+const Node *Parent = CommonAnc->Parent;
+// If parent is a DeclStmt, even though it's unselected, we consider it a
+// root statement and return its parent.
why ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
why is this function returning a shared_ptr ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+ if (WithTypeAndQualifiers) {
+if (IsConst)
+ Result += "const ";
why don't we infer this const from QualType ?
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:347
+ZoneRelative DeclaredIn;
+// Stores the numbering of this declaration(i for the i-th Decl)
+unsigned DeclIndex;
what does `numbering` mean ?
=
[PATCH] D65526: [Clangd] First version of ExtractFunction
SureYeaah updated this revision to Diff 217108.
SureYeaah added a comment.
Fixed typo
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 = &a; FOO foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct FOO {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, struct FOO &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b; int *ptr = &a; 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 &c) {
+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,591 @@
+//===--- 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 &a) {
+// if(a < 5)
+// a = 5;
+// }
+// void f(int a) {
+// extracted(a);
+// }
+/
[PATCH] D65526: [Clangd] First version of ExtractFunction
SureYeaah updated this revision to Diff 216831.
SureYeaah marked 2 inline comments as done.
SureYeaah edited the summary of this revision.
SureYeaah added a comment.
Removed extra code
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 = &a; FOO foo;
+ [[a += foo.x + b;
+ *ptr++;]]
+})cpp";
+ std::string ParameterCheckOutput = R"cpp(
+struct FOO {
+ int x;
+};
+void extracted(int &a, int &b, int * &ptr, struct FOO &foo) {
+a += foo.x + b;
+ *ptr++;
+}
+void f(int a) {
+ int b; int *ptr = &a; 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 &c) {
+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,592 @@
+//===--- 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 &a)
[PATCH] D65526: [Clangd] First version of ExtractFunction
SureYeaah marked an inline comment as done.
SureYeaah added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:360
+
+CapturedSourceInfo::DeclInformation &
+CapturedSourceInfo::getDeclInformationFor(const Decl *D) {
SureYeaah wrote:
> kadircet wrote:
> > why not move this and 4 following functions into astvisitor?
> Because we would need to pass CapturedSourceInfo and ExtractionZone to the
> Visitor then?
Ignore.
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] First version of ExtractFunction
SureYeaah added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:214
+bool IsConst;
+Parameter(std::string Name, std::string Type, bool IsConst)
+: Name(Name), Type(Type), IsConst(IsConst) {}
sammccall wrote:
> I'd suggest capturing the type as a QualType instead of a string + const +
> ref flag
>
> When types may need to be re-spelled, we'll need that extra information - the
> context needed to re-spell is available at render() time, not addParameter()
> time.
Const will also depend on CapturedSourceInfo. So I'm passing QualType instead
of the name as string.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:360
+
+CapturedSourceInfo::DeclInformation &
+CapturedSourceInfo::getDeclInformationFor(const Decl *D) {
kadircet wrote:
> why not move this and 4 following functions into astvisitor?
Because we would need to pass CapturedSourceInfo and ExtractionZone to the
Visitor then?
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
