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