[PATCH] [C++11] Support for capturing of variable length arrays in lambda expression.
Richard Smith
richard at metafoo.co.uk
Tue Jul 22 20:56:15 PDT 2014
Thanks, this looks much nicer.
This needs more testcases, covering at least:
* serialization,
* generic lambdas,
* capturing multiple array bounds from a single variable,
* referencing the same VLA type multiple times in a single lambda-expression,
* applying sizeof to a captured VLA type within a lambda expression.
================
Comment at: include/clang/AST/Decl.h:2259-2260
@@ +2258,4 @@
+ Expr *getCapturedVLABoundExpr() const {
+ return hasCapturedVLABoundExpr() ? InitializerOrBitWidth.getPointer()
+ : nullptr;
+ }
----------------
Get the `Expr` from the `VariableArrayType` here.
================
Comment at: include/clang/AST/Decl.h:2264
@@ +2263,3 @@
+ /// field.
+ void setCapturedVLABoundExpr(Expr *CapturedExpr);
+
----------------
Pass in the `VariableArrayType` instead of its `Expr` here.
================
Comment at: include/clang/AST/Decl.h:2260-2266
@@ +2259,9 @@
+ /// \brief Get the captured variable length array type.
+ const Type *getCapturedVLAType() const {
+ return hasCapturedVLAType()
+ ? static_cast<const Type *>(InitializerOrBitWidth.getPointer())
+ : nullptr;
+ }
+ /// \brief Set the captured variable length array type for this field.
+ void setCapturedVLAType(const Type *VLAType);
+
----------------
Can you use `VariableArrayType*` instead of just `Type*` here?
================
Comment at: include/clang/Sema/ScopeInfo.h:405-409
@@ -404,1 +404,7 @@
+ Capture(SourceLocation Loc, SourceLocation EllipsisLoc,
+ QualType CaptureType, const Type *Tpy)
+ : VarAndNested(nullptr, /*isNested*/ false),
+ InitExprAndCaptureKind(const_cast<Type *>(Tpy), Cap_ByCopy), Loc(Loc),
+ EllipsisLoc(EllipsisLoc), CaptureType(CaptureType) {}
+
----------------
This seems unnecessary; the `Capture` does not need to know the type.
================
Comment at: include/clang/Sema/ScopeInfo.h:433
@@ -426,2 +432,3 @@
}
- bool isNested() { return VarAndNested.getInt(); }
+ bool isTypeCapture() const {
+ return InitExprAndCaptureKind.getInt() == Cap_ByCopy &&
----------------
Please call this `isVLACapture()` so that a reader can tell what it's for.
================
Comment at: include/clang/Sema/ScopeInfo.h:496
@@ -480,1 +495,3 @@
+ void addVLATypeCapture(SourceLocation Loc, SourceLocation EllipsisLoc,
+ QualType CaptureType, const Type *Tpy) {
----------------
There cannot be an ellipsis, because a VLA capture is always implicit.
================
Comment at: lib/CodeGen/CGExprCXX.cpp:1781-1791
@@ +1780,13 @@
+ if (CurField->hasCapturedVLABoundExpr()) {
+ assert((*i)
+ ->IgnoreImpCasts()
+ ->getType()
+ ->getPointeeType()
+ ->isVariableArrayType() &&
+ "Captured expression is not a binary operator.");
+ auto QArrayType = cast<VariableArrayType>((*i)
+ ->IgnoreImpCasts()
+ ->getType()
+ ->getPointeeType()
+ ->getAsArrayTypeUnsafe());
+ EmitStoreThroughLValue(RValue::get(VLASizeMap[QArrayType->getSizeExpr()]),
----------------
Directly grab the `VariableArrayType` from `CurField` here.
================
Comment at: lib/CodeGen/CGExprCXX.cpp:1815
@@ +1814,3 @@
+ "Captured type is not a variable length array type.");
+ auto QArrayType = cast<VariableArrayType>(
+ CurField->getCapturedVLAType()->getAsArrayTypeUnsafe());
----------------
This `QBlah` naming convention is not one we use. We'd usually call this `VAT` or similar.
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:675-680
@@ +674,8 @@
+ SourceLocation()).getScalarVal();
+ auto QArrayType =
+ cast<VariableArrayType>(FD->getCapturedVLABoundExpr()
+ ->IgnoreImpCasts()
+ ->getType()
+ ->getPointeeType()
+ ->getAsArrayTypeUnsafe());
+ VLASizeMap[QArrayType->getSizeExpr()] = ExprArg;
----------------
... and here
================
Comment at: lib/Sema/SemaDecl.cpp:9774
@@ -9768,1 +9773,3 @@
+ : SourceLocation(),
+ I->getType(), I->getCapturedVLABoundExpr());
}
----------------
You shouldn't need to pass in an expression here: CodeGen initializes the field itself, and in any case, this expression is wrong, because you're not allowed to evaluate the VLA size expression again at the point where the closure object is created.
================
Comment at: lib/Sema/SemaExpr.cpp:12216-12217
@@ -12218,3 +12215,4 @@
// Otherwise, evaluate and record it.
- if (Expr *Size = Vat->getSizeExpr()) {
- MarkDeclarationsReferencedInExpr(Size);
+ if (auto Size = VAT->getSizeExpr()) {
+ if (auto LSI = dyn_cast<LambdaScopeInfo>(CSI)) {
+ auto Loc = Size->getExprLoc();
----------------
You shouldn't do this if `LSI` has already captured this VLA type.
================
Comment at: lib/Sema/SemaExpr.cpp:12219
@@ +12218,3 @@
+ auto Loc = Size->getExprLoc();
+ auto QType = Size->getType();
+ auto Lambda = LSI->Lambda;
----------------
This seems like the wrong type; VLA bounds are always implicitly converted to `size_t`, but we don't model that conversion in the size expression. I'd suggest just always using `Context.getSizeType()` here. Otherwise you'll get asserts in CodeGen because its widened type doesn't match the captured field type.
================
Comment at: lib/Sema/SemaExpr.cpp:12233
@@ +12232,3 @@
+
+ LSI->addVLATypeCapture(Loc, Loc, QType, VAT);
+ } else {
----------------
This doesn't seem like a good location for the capture; `ExprLoc` would be better.
================
Comment at: lib/Sema/SemaExpr.cpp:12220-12227
@@ +12219,10 @@
+ if (auto LSI = dyn_cast<LambdaScopeInfo>(CSI)) {
+ auto Loc = Size->getExprLoc();
+ auto QType = Size->getType();
+ auto QArrayType = Context.getPointerType(QualType(Vat, 0));
+ auto TInfo = Context.getTrivialTypeSourceInfo(QType, Loc);
+ ExprResult CapExpr =
+ new (Context) CXXNullPtrLiteralExpr(QArrayType, Loc);
+ auto CastKind = PrepareScalarCast(CapExpr, QType);
+ CapExpr = ImpCastExprToType(CapExpr.get(), QType, CastKind);
+ auto Lambda = LSI->Lambda;
----------------
Delete this. Instead, just set the captured VLA type on the `FieldDecl`.
================
Comment at: lib/Sema/SemaExpr.cpp:12239-12241
@@ +12238,5 @@
+
+ LSI->addCapture(/*Var=*/nullptr, /*IsBlock=*/false,
+ /*isByref=*/false, /*isNested=*/false, Loc, Loc,
+ QType, CapExpr.get());
+ } else {
----------------
Likewise, you shouldn't need to pass in `CapExpr` here.
================
Comment at: test/CodeGenCXX/instantiate-typeof-vla.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++11 -emit-pch -o %t
----------------
Please name this test as something to do with VLAs and lambdas. `__typeof` is incidental here.
http://reviews.llvm.org/D4368
More information about the cfe-commits
mailing list