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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Haven't looked at everything yet, but wanted to mention one thing I noticed.



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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+        if (InsertionPoint->Parent->ASTNode.get<ParmVarDecl>() != nullptr) {
+          return false;
+        }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141757