[PATCH] D58164: Block+lambda: allow reference capture

This revision was automatically updated to reflect the committed changes. Closed by commit rGe4dfc9f5bda3: Fix the type of the capture passed to LambdaIntroducer::addCapture in… (authored by ahatanak). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, I think this is a reasonable fix. If @rsmith has thoughts about this, I guess they'll have to happen post-review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak updated this revision to Diff 248371. ahatanak changed the repository for this revision from rC Clang to rG LLVM Github Monorepo. ahatanak added a comment. Herald added a subscriber: ributzka. Rebase and ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak updated this revision to Diff 212016. ahatanak added a comment. Rebase. I think the fix is correct. When the lambda expression for the generic lambda is built, `BuildLambdaExpr` passes a `Capture` object in `LambdaScopeInfo::Captures` to `BuildCaptureField` to build the closure class

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall added a comment. Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.llvm.org/D58164

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak added a comment. Currently a block captures a variable (POD or non-POD) by reference if the enclosing lambda captures it by reference and captures by copy if the enclosing lambda captures by copy. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall added a comment. I'm not sure we've ever written down what the semantics of the block capture are actually supposed to be in this situation. I guess capturing a reference is the right thing to do? Is that what we generally do if this is a POD type? Repository: rC Clang CHANGES

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak added a reviewer: rsmith. ahatanak added a comment. Richard, could you shed light on why it's done this way? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.llvm.org/D58164 ___

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak added a comment. I think I now have a better idea of what's causing the crash in IRGen. The root of the problem is that, when `RebuildLambdaScopeInfo` is called to rebuild the scope info for the generic lambda, the type of the captured variable (`s` in `test2` and `test3` in

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak updated this revision to Diff 204882. ahatanak added a comment. - Add another test case which has a block nested inside a lambda and causes clang to crash. - Fix the capture type passed to `addCapture` in `RebuildLambdaScopeInfo`. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall added a comment. I agree. There should just never be a copy expression if the capture is of reference type, and there should therefore be no need to special-case that in IRGen. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak requested changes to this revision. ahatanak added a comment. This revision now requires changes to proceed. Sorry, I didn't mean to accept this yet. I think this is something that is better fixed in Sema. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak accepted this revision. ahatanak added a comment. This revision is now accepted and ready to land. Sorry, I was misunderstanding the problem. I was trying to understand why the crash goes away if I change the generic lambda to a non-generic one. What I found was that, when the lambda

[PATCH] D58164: Block+lambda: allow reference capture

jfb added a comment. I talked to Akira in meatspace, and it seems like this updated patch does the right thing. He suggested changing the AST as a longer-term solution, but for now this approach seems simple enough. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58164: Block+lambda: allow reference capture

jfb updated this revision to Diff 186752. jfb added a comment. - Check for references when looking for copyexpr directly. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.llvm.org/D58164 Files: lib/CodeGen/CGBlocks.cpp

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak added a comment. I think the root of the problem is that `BlockDecl` knows the captured variable but doesn't know the type of the capture. The type of the variable and the type of the capture can be different if the block is nested inside a lambda or another block, which is why

[PATCH] D58164: Block+lambda: allow reference capture

ahatanak added a comment. The code is crashing here because the loop in `computeBlockInfo` is trying to capture a variable that is captured by reference by the enclosing lambda as if it were captured by value. This is the type of VT: LValueReferenceType 0x1138008c0 'struct derp &'

[PATCH] D58164: Block+lambda: allow reference capture

jfb created this revision. jfb added reviewers: rjmccall, ahatanak. Herald added subscribers: cfe-commits, dexonsmith, jkorous. Herald added a project: clang. Capturing a C++ object by reference wasn't quite working when mixing block and lambda. Repository: rC Clang