llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Christian Kandeler (ckandeler)
<details>
<summary>Changes</summary>
The tweak refused to trigger whenever the extraction zone contained a single
statement that was an expression, e.g. a lone call like `log("connection
failed");` or an overloaded-operator statement like `std::cout << "x";`.
The former was blocked by an overly broad check in validSingleChild(); the
latter additionally required getParentOfRootStmts() to recognize that such a
statement can be its own root statement even while marked Unselected, rather
than being treated as a container of root statements.
Fixes #<!-- -->698
Fixes #<!-- -->1254
Assisted-by: Claude
---
Full diff: https://github.com/llvm/llvm-project/pull/219945.diff
3 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
(+23-12)
- (modified) clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
(+33-8)
- (modified) clang-tools-extra/docs/ReleaseNotes.md (+5)
``````````diff
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index bc9a790232507..624b11094ef09 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -95,6 +95,15 @@ enum FunctionDeclKind {
OutOfLineDefinition
};
+// Whether N, despite being Unselected, may still be a single RootStmt: a
+// DeclStmt can be unselected since VarDecls claim the entire selection range
+// in the selection tree. Similarly, a CXXOperatorCallExpr of a binary
+// operation can be unselected because its children (the operands) claim the
+// entire selection range in the selection tree (e.g. <<).
+bool isUnselectedRootStmtCandidate(const Node *N) {
+ return N->ASTNode.get<DeclStmt>() || N->ASTNode.get<CXXOperatorCallExpr>();
+}
+
// A RootStmt is a statement that's fully selected including all its children
// and its parent is unselected.
// Check if a node is a root statement.
@@ -104,12 +113,8 @@ bool isRootStmt(const Node *N) {
// Root statement cannot be partially selected.
if (N->Selected == SelectionTree::Partial)
return false;
- // A DeclStmt can be an unselected RootStmt since VarDecls claim the entire
- // selection range in selectionTree. Additionally, a CXXOperatorCallExpr of a
- // binary operation can be unselected because its children claim the entire
- // selection range in the selection tree (e.g. <<).
- if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>()
&&
- !N->ASTNode.get<CXXOperatorCallExpr>())
+ if (N->Selected == SelectionTree::Unselected &&
+ !isUnselectedRootStmtCandidate(N))
return false;
return true;
}
@@ -130,7 +135,18 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) {
const Node *Parent = nullptr;
switch (CommonAnc->Selected) {
case SelectionTree::Selection::Unselected:
- // Typically a block, with the { and } unselected, could also be ForStmt
etc
+ // Typically a block, with the { and } unselected, could also be ForStmt
+ // etc. However, CommonAnc may instead be a single statement that is
+ // itself Unselected only because all of its own tokens are claimed by
+ // its children (see isUnselectedRootStmtCandidate); in that case it's a
+ // root statement in its own right, and we need its actual parent, same
+ // as in the Complete case below.
+ if (isUnselectedRootStmtCandidate(CommonAnc)) {
+ Parent = CommonAnc->Parent;
+ if (Parent->ASTNode.get<DeclStmt>())
+ Parent = Parent->Parent;
+ break;
+ }
// Ensure all Children are RootStmts.
Parent = CommonAnc;
break;
@@ -298,11 +314,6 @@ computeEnclosingFuncRange(const FunctionDecl
*EnclosingFunction,
// returns true if Child can be a single RootStmt being extracted from
// EnclosingFunc.
bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) {
- // Don't extract expressions.
- // FIXME: We should extract expressions that are "statements" i.e. not
- // subexpressions
- if (Child->ASTNode.get<Expr>())
- return false;
// Extracting the body of EnclosingFunc would remove it's definition.
assert(EnclosingFunc->hasBody() &&
"We should always be extracting from a function body.");
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
index eff4d0f43595c..edad883950071 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
@@ -24,8 +24,8 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
// 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");
+ // Single expression-statements can be extracted.
+ EXPECT_THAT(apply("int x = 0; [[x++;]]"), HasSubstr("extracted"));
// We don't support extraction from lambdas.
EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
// Partial statements aren't extracted.
@@ -192,16 +192,16 @@ F (extracted();)
EXPECT_EQ(apply(CompoundFailInput), "unavailable");
ExtraArgs.push_back("-std=c++14");
- // FIXME: Expressions are currently not extracted
- EXPECT_EQ(apply(R"cpp(
+ // A bare expression-statement can be extracted (the semicolon isn't part
+ // of the selection either way, since it isn't owned by any AST node).
+ EXPECT_THAT(apply(R"cpp(
void call() { [[1+1]]; }
)cpp"),
- "unavailable");
- // FIXME: Single expression statements are currently not extracted
- EXPECT_EQ(apply(R"cpp(
+ HasSubstr("extracted"));
+ EXPECT_THAT(apply(R"cpp(
void call() { [[1+1;]] }
)cpp"),
- "unavailable");
+ HasSubstr("extracted"));
}
TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
@@ -630,6 +630,31 @@ int main() {
EXPECT_EQ(apply(Before), After);
}
+TEST_F(ExtractFunctionTest, SingleStatement) {
+ Context = File;
+ // https://github.com/clangd/clangd/issues/698
+ // A single call-expression-statement can be extracted.
+ EXPECT_THAT(apply(R"cpp(
+ void foo(int, int);
+ void bar() {
+ [[foo(1, 2);]]
+ })cpp"),
+ HasSubstr("extracted"));
+ // https://github.com/clangd/clangd/issues/1254
+ // A single statement consisting of an overloaded binary operator call can
+ // be extracted, even though the SelectionTree marks the
+ // CXXOperatorCallExpr itself as Unselected (its operands claim all the
+ // characters).
+ EXPECT_THAT(apply(R"cpp(
+ struct Stream {};
+ Stream &operator<<(Stream &, const char *);
+ Stream stream;
+ int main() {
+ [[stream << "x";]]
+ })cpp"),
+ HasSubstr("extracted"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/docs/ReleaseNotes.md
b/clang-tools-extra/docs/ReleaseNotes.md
index 633418a2abb98..ba00888b1adb3 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -82,6 +82,11 @@ infrastructure are described first, followed by
tool-specific sections.
- clangd now applies clang-tidy fix-it post-processing before exposing fixes.
+- The `Extract to function` tweak is now offered for selections consisting of
+ a single expression-statement (e.g. a lone function call or an overloaded
+ operator call such as `stream << 42;`), which it previously refused to
+ extract.
+
#### Signature help
#### Cross-references
``````````
</details>
https://github.com/llvm/llvm-project/pull/219945
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
