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

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti marked 4 inline comments as done.
5chmidti added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+    // Local variables declared inside of the selected lambda cannot go out of
+    // scope. The DeclRefExprs that are important are the variables captured and
----------------
nridge wrote:
> 5chmidti wrote:
> > nridge wrote:
> > > Looking at [RecursiveASTVisitor::TraverseLambdaExpr](https://searchfox.org/llvm/rev/094539cfcb46f55824402565e5c18580df689a67/clang/include/clang/AST/RecursiveASTVisitor.h#2652-2691), there are a few things it traverses in addition to the lambda's parameters and body (which we are saying are ok to skip) and the lambda's captures (which we are traversing).
> > > 
> > > For example, the lambda may have an explicit result type which references a variable in scope:
> > > 
> > > ```
> > > int main() {
> > >   if (int a = 1)
> > >     if ([[ []() -> decltype(a){ return 1; } ]] () == 4)
> > >       a = a + 1;
> > > }
> > > 
> > > ```
> > > 
> > > Here, extracting the lambda succeeds, and the reference to `a` in `decltype(a)` becomes dangling.
> > I'll update the diff when I've implemented this. A requires clause may reference a variable like `a` as well.
> > A requires clause may reference a variable like `a` as well.
> 
> Technically, an explicit template parameter list can also reference a local variable via e.g. a default argument for a non-type parameter.
> 
> I appreciate that we're getting into increasingly obscure edge cases here, so please feel free to use your judgment and draw a line somewhere; the refactoring introducing a compiler error when invoked on some obscure/unlikely piece of code is not that big of a deal.
I have added support for attributes, trailing return-type decls and explicit template parameters to the visitor. I think that is every edge case covered.


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


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:149
+      [](int x = [[10]]){};
+      [](auto h = [i = [[ [](){} ]]](){}) {};
+      [](auto h = [[ [i = [](){}](){} ]]) {};
----------------
nridge wrote:
> It's easy to overlook the purpose of this line amidst all the brackets, I would suggest adding a comment like:
> 
> ```
> // Extracting from a capture initializer is usually fine, but not if
> // the capture itself is nested inside a default argument
> ```
I added some comments like the one for this example to indicate why an extraction is expected to be unavailable. The same goes for the tests just above (ln134-189).


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

https://reviews.llvm.org/D141757