Author: Kadir Cetinkaya Date: 2020-10-14T14:22:47+02:00 New Revision: 82a71822a54d76c62bf730d8c0e8e86d68c60159
URL: https://github.com/llvm/llvm-project/commit/82a71822a54d76c62bf730d8c0e8e86d68c60159 DIFF: https://github.com/llvm/llvm-project/commit/82a71822a54d76c62bf730d8c0e8e86d68c60159.diff LOG: [clangd] Disable extract variable for RHS of assignments Differential Revision: https://reviews.llvm.org/D89307 Added: Modified: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp clang-tools-extra/clangd/unittests/TweakTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp index 8b668be5f2f9..8feef4e84722 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp @@ -382,17 +382,27 @@ bool eligibleForExtraction(const SelectionTree::Node *N) { if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind)) return false; + const SelectionTree::Node &OuterImplicit = N->outerImplicit(); + const auto *Parent = OuterImplicit.Parent; + if (!Parent) + return false; // We don't want to extract expressions used as statements, that would leave // a `dummy;` around that has no effect. // Unfortunately because the AST doesn't have ExprStmt, we have to check in // this roundabout way. - const SelectionTree::Node &OuterImplicit = N->outerImplicit(); - if (!OuterImplicit.Parent || - childExprIsStmt(OuterImplicit.Parent->ASTNode.get<Stmt>(), + if (childExprIsStmt(Parent->ASTNode.get<Stmt>(), OuterImplicit.ASTNode.get<Expr>())) return false; - // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]` + // Disable extraction of full RHS on assignment operations, e.g: + // auto x = [[RHS_EXPR]]; + // This would just result in duplicating the code. + if (const auto *BO = Parent->ASTNode.get<BinaryOperator>()) { + if (BO->isAssignmentOp() && + BO->getRHS() == OuterImplicit.ASTNode.get<Expr>()) + return false; + } + return true; } diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index f64d42a7eed7..718b84d03990 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -215,26 +215,26 @@ TEST_F(ExtractVariableTest, Test) { int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1; // if without else if([[1]]) - a = [[1]]; + a = [[1]] + 1; // if with else if(a < [[3]]) if(a == [[4]]) - a = [[5]]; + a = [[5]] + 1; else - a = [[5]]; + a = [[5]] + 1; else if (a < [[4]]) - a = [[4]]; + a = [[4]] + 1; else - a = [[5]]; + a = [[5]] + 1; // for loop - for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++) - a = [[2]]; + for(a = [[1]] + 1; a > [[[[3]] + [[4]]]]; a++) + a = [[2]] + 1; // while while(a < [[1]]) - a = [[1]]; + a = [[1]] + 1; // do while do - a = [[1]]; + a = [[1]] + 1; while(a < [[3]]); } )cpp"; @@ -291,6 +291,7 @@ TEST_F(ExtractVariableTest, Test) { xyz([[a *= 5]]); // Variable DeclRefExpr a = [[b]]; + a = [[xyz()]]; // statement expression [[xyz()]]; while (a) @@ -373,10 +374,10 @@ TEST_F(ExtractVariableTest, Test) { })cpp"}, // attribute testing {R"cpp(void f(int a) { - [ [gsl::suppress("type")] ] for (;;) a = [[1]]; + [ [gsl::suppress("type")] ] for (;;) a = [[1]] + 1; })cpp", R"cpp(void f(int a) { - auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy; + auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy + 1; })cpp"}, // MemberExpr {R"cpp(class T { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits