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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to