Richard, here are my comments. The updated patch is ready and I'll upload it
soon.
================
Comment at: include/clang/AST/LambdaCapture.h:67
@@ -66,3 +66,3 @@
/// \brief Determine the kind of capture.
LambdaCaptureKind getCaptureKind() const;
----------------
rsmith wrote:
> Have you considered adding an LCK_ value for VLA capture? Calling it
> `LCK_ByCopy` doesn't seem quite right.
Yes, I tried this. But the problem is that there is no more space for another
one LCK_ value in field DeclAndBits. It is declared as
llvm::PointerIntPair<Decl *, 2> and its integer argument may have values
Capture_Implicit = 0x01 or Capture_ByCopy = 0x02. I can't add vallue 0x03v
without changing llvm::PointerIntPair<Decl *, 2> to llvm::PointerIntPair<Decl
*, 3>. The only way to do it is to split DeclAndBits into 2 independent fields
- pointer and flags.
================
Comment at: lib/AST/Decl.cpp:3303-3305
@@ -3292,4 +3302,5 @@
void FieldDecl::setBitWidth(Expr *Width) {
+ assert(isBitField() && "not a bitfield");
assert(!InitializerOrBitWidth.getPointer() && !hasInClassInitializer() &&
- "bit width or initializer already set");
+ "bit width, initializer or captured type already set");
InitializerOrBitWidth.setPointer(Width);
----------------
rsmith wrote:
> This pair of asserts doesn't make sense. A field can only be a bit-field if
> it has a bit-width, but you're asserting that it's a bit-field *and* has no
> bit-width.
>
> This is only passing the tests because `setBitWidth` is never called. Please
> remove the assert you added here. (I suspect this function is called by lldb
> or similar, otherwise I'd just suggest removing it entirely.)
Yers, you're right. I'll remove assert. The method is used by some tools and I
think we should keep it.
================
Comment at: lib/AST/Decl.cpp:3317
@@ +3316,3 @@
+ return getDeclContext()->isRecord() && getParent()->isLambda() &&
+ getInClassInitStyle() == ICIS_NoInit &&
+ InitializerOrBitWidth.getPointer();
----------------
rsmith wrote:
> Is this check necessary? You can't have an in-class initializer in a lambda.
No, it is not neccessary, just double checked some conditions. I think it can
be removed.
================
Comment at: lib/AST/Decl.cpp:3549-3551
@@ +3548,5 @@
+bool RecordDecl::isLambda() const {
+ if (auto RD = dyn_cast<CXXRecordDecl>(this)) {
+ return RD->isLambda();
+ }
+ return false;
----------------
rsmith wrote:
> No braces here, please.
Removed
================
Comment at: lib/AST/StmtPrinter.cpp:1711-1712
@@ -1710,2 +1710,4 @@
++C) {
+ if (C->capturesVLAType())
+ continue;
if (NeedComma)
----------------
rsmith wrote:
> Is this possible? I would have expected VLA captures to always be implicit.
You're right, removed.
================
Comment at: lib/AST/StmtProfile.cpp:1020-1021
@@ -1019,2 +1019,4 @@
C != CEnd; ++C) {
+ if (C->capturesVLAType())
+ continue;
ID.AddInteger(C->getCaptureKind());
----------------
rsmith wrote:
> Likewise.
Removed too.
================
Comment at: lib/Sema/SemaExpr.cpp:11668
@@ -11667,2 +11667,3 @@
// Prohibit variably-modified types; they're difficult to deal with.
+ if (Var->getType()->isVariablyModifiedType() && IsBlock) {
----------------
rsmith wrote:
> Update this comment: "Prohibit variably-modified types in blocks; ..." maybe?
Done.
================
Comment at: lib/Sema/SemaExpr.cpp:12075-12082
@@ -12077,2 +12074,10 @@
+static bool isVLATypeIsCaptured(LambdaScopeInfo *LSI,
+ const VariableArrayType *VAT) {
+ for (auto *FD : LSI->Lambda->fields()) {
+ if (FD->hasCapturedVLAType() && FD->getCapturedVLAType() == VAT)
+ return true;
+ }
+ return false;
+}
----------------
rsmith wrote:
> Please make this a member of `CapturingScopeInfo`. Also, drop one of the
> `Is`s from its name.
Ok
http://reviews.llvm.org/D4368
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
