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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
