This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Disallow extraction of expression-statements.
ClosedPublic

Authored by sammccall on Jul 26 2019, 10:08 AM.

Details

Summary

I split out the "extract parent instead of this" logic from the "this isn't
worth extracting" logic (now in eligibleForExtraction()), because I found it
hard to reason about.

While here, handle overloaded as well as builtin assignment operators.

Also this uncovered a bug in getCallExpr() which I fixed.

Event Timeline

sammccall created this revision.Jul 26 2019, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 10:08 AM

What was the bug in getCallExpr() ?

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

Remove dump.

445

Check if parent is an assignment binaryoperator or a vardecl?

461

Can we combine both these IFs and remove the unused assignment?

SureYeaah added inline comments.Jul 29 2019, 6:06 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
329

Change to a=[[1]];?

sammccall marked 6 inline comments as done.Jul 30 2019, 1:54 PM

What was the bug in getCallExpr() ?

It could find calls where the DeclRef was an arbitrary subexpression of the callee, not exactly the callee (modulo implicit nodes).

It allowed extraction of [[t]].bar(t.z), because it recognized t as a DeclRefExpr (maybe a function we're calling!) and then walked up the stack looking for a call.
It found one, and verified that the callee was the top of the stack (which was [[t.bar]](t.z) at that point).

In the previous version of the code, this was filtered out by one of the redundant checks, but after removing them the problem showed up. Once here I decided to remove the stack (only the top element was ever used) and reuse outerImplicit since we had it anyway and it avoided explicit looping.

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

Right. I want to keep that out of this patch though: more tests, more changes to existing tests, more risk that it's not exactly what we want (e.g. extraction of *constants* to a named variable might be fine...)

461

Done. Also removed the clobbering of TargetNode if there's no call, this decision is already covered by eligibleForExtraction. And then cleaned up some widely-scoped variables and impossible conditions here.

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

Sure, though the next patch may disallow this too :-)

sammccall updated this revision to Diff 212432.Jul 30 2019, 1:55 PM
sammccall marked 2 inline comments as done.

Address review comments.

sammccall updated this revision to Diff 212433.Jul 30 2019, 1:56 PM

Add basic test for outerImplicit.

Harbormaster completed remote builds in B35837: Diff 212433.
SureYeaah accepted this revision.Aug 6 2019, 6:29 AM
This revision is now accepted and ready to land.Aug 6 2019, 6:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 4:43 PM