diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp index 79bf962544242..3b378153eafd5 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp @@ -468,7 +468,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(); @@ -483,13 +484,48 @@ bool eligibleForExtraction(const SelectionTree::Node *N) { OuterImplicit.ASTNode.get())) return false; + std::function IsFullySelected = + [&](const SelectionTree::Node *N) { + if (N->ASTNode.getSourceRange().isValid() && + N->Selected != SelectionTree::Complete) + return false; + for (const auto *Child : N->Children) { + if (!IsFullySelected(Child)) + return false; + } + return true; + }; + auto ExprIsFullySelectedTargetNode = [&](const Expr *E) { + if (E != OuterImplicit.ASTNode.get()) + return false; + + // The above condition is the only relevant one except for binary operators. + // Without the following code, we would fail to offer extraction for e.g.: + // int x = 1 + 2 + [[3 + 4 + 5]]; + // See the documentation of ParsedBinaryOperator for further details. + if (!IsBinOp) + return true; + return IsFullySelected(N); + }; + // Disable extraction of full RHS on assignment operations, e.g: - // auto x = [[RHS_EXPR]]; + // x = [[RHS_EXPR]]; // This would just result in duplicating the code. if (const auto *BO = Parent->ASTNode.get()) { - if (BO->isAssignmentOp() && - BO->getRHS() == OuterImplicit.ASTNode.get()) + if (BO->isAssignmentOp() && ExprIsFullySelectedTargetNode(BO->getRHS())) + return false; + } + + // The same logic as for assignments applies to initializations. + // However, we do allow extracting the RHS of an init capture, as it is + // a valid use case to move non-trivial expressions out of the capture clause. + // FIXME: In that case, the extracted variable should be captured directly, + // rather than an explicit copy. + if (const auto *Decl = Parent->ASTNode.get()) { + if (!Decl->isInitCapture() && + ExprIsFullySelectedTargetNode(Decl->getInit())) { 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 eb5b06cfee431..42dd612eeeec4 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); @@ -79,7 +79,7 @@ TEST_F(ExtractVariableTest, Test) { @end @implementation Foo - (void)method { - int x = [[1 + 2]]; + int x = [[1]] + 2; } @end)cpp"; EXPECT_AVAILABLE(AvailableObjC); @@ -103,6 +103,9 @@ TEST_F(ExtractVariableTest, Test) { } int z = [[1]]; } t; + int x = [[1 + 2]]; + int y; + y = [[1 + 2]]; return [[t]].bar([[t]].z); } void v() { return; } @@ -430,8 +433,8 @@ 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(void f() { auto x = +[[ [](){ return 42; }]]; })cpp", + R"cpp(void f() { auto placeholder = [](){ return 42; }; auto x = + placeholder; })cpp"}, {R"cpp( template auto sink(T f) { return f(); } @@ -515,13 +518,13 @@ TEST_F(ExtractVariableTest, Test) { {R"cpp( template void foo(Ts ...args) { - auto x = [[ [&args...]() {} ]]; + auto x = +[[ [&args...]() {} ]]; } )cpp", R"cpp( template void foo(Ts ...args) { - auto placeholder = [&args...]() {}; auto x = placeholder ; + auto placeholder = [&args...]() {}; auto x = + placeholder ; } )cpp"}, {R"cpp( @@ -533,7 +536,7 @@ TEST_F(ExtractVariableTest, Test) { int main() { Coordinates c = {}; const auto [x, y] = c; - auto f = [[ [&]() { return x + y; } ]]; + auto f = [[ [&]() { return x + y; } ]](); } )cpp", R"cpp( @@ -545,7 +548,7 @@ TEST_F(ExtractVariableTest, Test) { int main() { Coordinates c = {}; const auto [x, y] = c; - auto placeholder = [&]() { return x + y; }; auto f = placeholder ; + auto placeholder = [&]() { return x + y; }; auto f = placeholder (); } )cpp"}, {R"cpp( @@ -557,7 +560,7 @@ TEST_F(ExtractVariableTest, Test) { int main() { Coordinates c = {}; if (const auto [x, y] = c; x > y) { - auto f = [[ [&]() { return x + y; } ]]; + auto f = [[ [&]() { return x + y; } ]](); } } )cpp", @@ -570,7 +573,7 @@ TEST_F(ExtractVariableTest, Test) { int main() { Coordinates c = {}; if (const auto [x, y] = c; x > y) { - auto placeholder = [&]() { return x + y; }; auto f = placeholder ; + auto placeholder = [&]() { return x + y; }; auto f = placeholder (); } } )cpp"},