Skip to content

Commit 82a7182

Browse files
committed
[clangd] Disable extract variable for RHS of assignments
Differential Revision: https://reviews.llvm.org/D89307
1 parent 73c6beb commit 82a7182

File tree

2 files changed

+26
-15
lines changed

2 files changed

+26
-15
lines changed

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -382,17 +382,27 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
382382
if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
383383
return false;
384384

385+
const SelectionTree::Node &OuterImplicit = N->outerImplicit();
386+
const auto *Parent = OuterImplicit.Parent;
387+
if (!Parent)
388+
return false;
385389
// We don't want to extract expressions used as statements, that would leave
386390
// a `dummy;` around that has no effect.
387391
// Unfortunately because the AST doesn't have ExprStmt, we have to check in
388392
// this roundabout way.
389-
const SelectionTree::Node &OuterImplicit = N->outerImplicit();
390-
if (!OuterImplicit.Parent ||
391-
childExprIsStmt(OuterImplicit.Parent->ASTNode.get<Stmt>(),
393+
if (childExprIsStmt(Parent->ASTNode.get<Stmt>(),
392394
OuterImplicit.ASTNode.get<Expr>()))
393395
return false;
394396

395-
// FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
397+
// Disable extraction of full RHS on assignment operations, e.g:
398+
// auto x = [[RHS_EXPR]];
399+
// This would just result in duplicating the code.
400+
if (const auto *BO = Parent->ASTNode.get<BinaryOperator>()) {
401+
if (BO->isAssignmentOp() &&
402+
BO->getRHS() == OuterImplicit.ASTNode.get<Expr>())
403+
return false;
404+
}
405+
396406
return true;
397407
}
398408

clang-tools-extra/clangd/unittests/TweakTests.cpp

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -215,26 +215,26 @@ TEST_F(ExtractVariableTest, Test) {
215215
int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
216216
// if without else
217217
if([[1]])
218-
a = [[1]];
218+
a = [[1]] + 1;
219219
// if with else
220220
if(a < [[3]])
221221
if(a == [[4]])
222-
a = [[5]];
222+
a = [[5]] + 1;
223223
else
224-
a = [[5]];
224+
a = [[5]] + 1;
225225
else if (a < [[4]])
226-
a = [[4]];
226+
a = [[4]] + 1;
227227
else
228-
a = [[5]];
228+
a = [[5]] + 1;
229229
// for loop
230-
for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
231-
a = [[2]];
230+
for(a = [[1]] + 1; a > [[[[3]] + [[4]]]]; a++)
231+
a = [[2]] + 1;
232232
// while
233233
while(a < [[1]])
234-
a = [[1]];
234+
a = [[1]] + 1;
235235
// do while
236236
do
237-
a = [[1]];
237+
a = [[1]] + 1;
238238
while(a < [[3]]);
239239
}
240240
)cpp";
@@ -291,6 +291,7 @@ TEST_F(ExtractVariableTest, Test) {
291291
xyz([[a *= 5]]);
292292
// Variable DeclRefExpr
293293
a = [[b]];
294+
a = [[xyz()]];
294295
// statement expression
295296
[[xyz()]];
296297
while (a)
@@ -373,10 +374,10 @@ TEST_F(ExtractVariableTest, Test) {
373374
})cpp"},
374375
// attribute testing
375376
{R"cpp(void f(int a) {
376-
[ [gsl::suppress("type")] ] for (;;) a = [[1]];
377+
[ [gsl::suppress("type")] ] for (;;) a = [[1]] + 1;
377378
})cpp",
378379
R"cpp(void f(int a) {
379-
auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy;
380+
auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy + 1;
380381
})cpp"},
381382
// MemberExpr
382383
{R"cpp(class T {

0 commit comments

Comments
 (0)