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