[PATCH] D141757: [clangd] allow extracting to variable for lambda expressions

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked an inline comment as done.
5chmidti added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+        // Allow expressions, but only allow completely selected lambda
+        // expressions or unselected lambda expressions that are the parent of
+        // the originally selected node, not partially selected lambda
----------------
nridge wrote:
> nridge wrote:
> > 5chmidti wrote:
> > > nridge wrote:
> > > > I think it's worth adding to the comment *why* we allow unselected lambda expressions (to allow extracting from capture initializers), as it's not obvious
> > > I changed the previous return of `!isa<LambdaExpr>(Stmt) || InsertionPoint->Selected != SelectionTree::Partial;` to a simple `return true;`. None of my partial selection tests fail and I can't think of a case where the lambda would be partially selected.
> > Hmm, so what actually happens in these testcases?
> > 
> > ```
> >       // lambdas: partially selected
> >       [][[(){}]];
> >       []()[[{}]];
> >       [ [[ ](){}]];
> > ```
> I checked, and in these cases we disallow the extraction [here](https://searchfox.org/llvm/rev/c2bee1ed26a3355d164c92f1eb70ebf88804560d/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp#426-432) before we get to creating an `ExtractionContext`.
> 
> If the testcases are modified as follows:
> 
> ```
> template <typename T> void sink(T) {}
> ...
> void f() {
>   // lambdas: partially selected
>   sink([][[(){}]]);
>   sink([]()[[{}]]);
>   sink([ [[ ](){}]]);
> }
> ```
> 
> now they fail, unless the check for partial selection here is restored.
As stated in the update comment, I see no reason why we actually block partial selection of lambdas. It's okay to extract from the initializer of a capture (one of your comments above):
```
int foo();

void bar() {
  [x = [[foo()]] ]() {};
}
```
The tests in ln 138-143 (titled `lambdas: captures`) check that we don't extract the wrong things from lambda captures.
Do you see a way that extracting from a lambda capture or any kind of partial selection could be problematic?

To answer the question anyway:
`Hmm, so what actually happens in these testcases?`
The original diff had a sink that I probably removed because the tests would succeed even without the sink. However, those tests then tested a different condition and never hit the check for partial selection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757