[PATCH] D65337: [clangd] Disallow extraction of expression-statements.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added a comment.

In D65337#1604324 <https://reviews.llvm.org/D65337#1604324>, @SureYeaah wrote:

> 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.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:445
+
+  // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
   return true;
----------------
SureYeaah wrote:
> Check if parent is an assignment binaryoperator or a vardecl?
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...)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:461
+  // For function and member function DeclRefs, extract the whole call.
+  if (const DeclRefExpr *DeclRef = dyn_cast_or_null<DeclRefExpr>(SelectedExpr))
     TargetNode = getCallExpr(N);
----------------
SureYeaah wrote:
> Can we combine both these IFs and remove the unused assignment?
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.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:329
       while(a < [[1]])
-        [[a++]];
+        a++;
       // do while
----------------
SureYeaah wrote:
> Change to a=[[1]];?
Sure, though the next patch may disallow this too :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65337/new/

https://reviews.llvm.org/D65337