https://github.com/ckandeler created
https://github.com/llvm/llvm-project/pull/69477
That would turn:
int x = f() + 1;
into:
auto placeholder = f() + 1;
int x = placeholder;
which makes little sense and is clearly not intended, as stated explicitly by a
comment in eligibleForExtraction(). It appears that the declaration case was
simply forgotten (the assignment case was already implemented).
>From 57ff6adb986821ce41d21f056dd6d1355d4ec06f Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kande...@qt.io>
Date: Wed, 18 Oct 2023 17:24:04 +0200
Subject: [PATCH] [clangd] Do not offer extraction to variable for decl init
expression
That would turn:
int x = f() + 1;
into:
auto placeholder = f() + 1;
int x = placeholder;
which makes little sense and is clearly not intended, as stated
explicitly by a comment in eligibleForExtraction(). It appears that the
declaration case was simply forgotten (the assignment case was already
implemented).
---
.../refactor/tweaks/ExtractVariable.cpp | 10 ++-
.../unittests/tweaks/ExtractVariableTests.cpp | 70 +------------------
2 files changed, 12 insertions(+), 68 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 1e4edc6d6b4bb39..26fa94ded1f012e 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -467,7 +467,8 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
// Extracting Exprs like a = 1 gives placeholder = a = 1 which isn't useful.
// FIXME: we could still hoist the assignment, and leave the variable there?
ParsedBinaryOperator BinOp;
- if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
+ bool IsBinOp = BinOp.parse(*N);
+ if (IsBinOp && BinaryOperator::isAssignmentOp(BinOp.Kind))
return false;
const SelectionTree::Node &OuterImplicit = N->outerImplicit();
@@ -490,6 +491,13 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
BO->getRHS() == OuterImplicit.ASTNode.get<Expr>())
return false;
}
+ if (const auto *Decl = Parent->ASTNode.get<VarDecl>()) {
+ if (!Decl->isInitCapture() &&
+ Decl->getInit() == OuterImplicit.ASTNode.get<Expr>() &&
+ (!IsBinOp || !BinOp.associative())) {
+ return false;
+ }
+ }
return true;
}
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index 3d74a941071f849..7ca717747e49a7b 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -27,10 +27,10 @@ TEST_F(ExtractVariableTest, Test) {
return [[[[t.b[[a]]r]]([[t.z]])]];
}
void f() {
- int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
+ int a = 5 + [[4 * [[[[xyz]]()]]]];
// multivariable initialization
if(1)
- int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
+ int x = [[1]] + 1, y = [[a + 1]], a = [[1]] + 2, z = a + 1;
// if without else
if([[1]])
a = [[1]] + 1;
@@ -61,7 +61,7 @@ TEST_F(ExtractVariableTest, Test) {
ExtraArgs = {"-xc"};
const char *AvailableC = R"cpp(
void foo() {
- int x = [[1]];
+ int x = [[1]] + 1;
})cpp";
EXPECT_AVAILABLE(AvailableC);
ExtraArgs = {"-xobjective-c"};
@@ -422,8 +422,6 @@ TEST_F(ExtractVariableTest, Test) {
int member = 42;
};
)cpp"},
- {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
- R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x =
placeholder; })cpp"},
{R"cpp(
template <typename T>
auto sink(T f) { return f(); }
@@ -504,68 +502,6 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
auto placeholder = []() { return 42; }(); return placeholder ;
})cpp"},
- {R"cpp(
- template <typename ...Ts>
- void foo(Ts ...args) {
- auto x = [[ [&args...]() {} ]];
- }
- )cpp",
- R"cpp(
- template <typename ...Ts>
- void foo(Ts ...args) {
- auto placeholder = [&args...]() {}; auto x = placeholder ;
- }
- )cpp"},
- {R"cpp(
- struct Coordinates {
- int x{};
- int y{};
- };
-
- int main() {
- Coordinates c = {};
- const auto [x, y] = c;
- auto f = [[ [&]() { return x + y; } ]];
- }
- )cpp",
- R"cpp(
- struct Coordinates {
- int x{};
- int y{};
- };
-
- int main() {
- Coordinates c = {};
- const auto [x, y] = c;
- auto placeholder = [&]() { return x + y; }; auto f = placeholder ;
- }
- )cpp"},
- {R"cpp(
- struct Coordinates {
- int x{};
- int y{};
- };
-
- int main() {
- Coordinates c = {};
- if (const auto [x, y] = c; x > y) {
- auto f = [[ [&]() { return x + y; } ]];
- }
- }
- )cpp",
- R"cpp(
- struct Coordinates {
- int x{};
- int y{};
- };
-
- int main() {
- Coordinates c = {};
- if (const auto [x, y] = c; x > y) {
- auto placeholder = [&]() { return x + y; }; auto f = placeholder ;
- }
- }
- )cpp"},
// 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
