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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+        if (InsertionPoint->Parent->ASTNode.get<ParmVarDecl>() != nullptr) {
+          return false;
+        }
----------------
nridge wrote:
> 5chmidti wrote:
> > This is supposed to stop the following invalid code from happening:
> > ```
> > void foo() {
> >   int placeholder = 42;
> >   [](int x = placeholder {};
> >   extern void bar(int x = placeholder);
> > }
> > ```
> > 
> > clangd does not seem to support extracting from the initializers of defaulted parameters, should I keep the condition as is, or should I do something different here? It is legal to have default arguments in global scope (examples below).
> > 
> > The following insertions could exist (out of scope for this patch):
> > ```
> > int placeholder = 42;
> > void foo() {
> >   [](int x = placeholder {};
> >   extern void bar(int x = placeholder);
> > }
> > ```
> > ```
> > struct X {
> >   static inline int placeholder = 42;
> >   void foo(int x = placeholder) {}
> > };
> > ```
> > 
> > Either way, I'll have to adjust the comment because this is not just to stop default parameter initializers in lambdas from being extracted to a local scope.
> I'm having trouble following this comment. Can you give the testcase that would produce this invalid code?
I provided test cases in lines 147-150 in ExtractVariableTests.cpp. For the example given in my comment, the test case would be:
Pre:
```
void foo() {
  [](int x = [[42]]) {};
}
```
(erroneous) Post:
```
void foo() {
  int placeholder = 42;
  [](int x = placeholder) {};
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757