rsmith added inline comments.
================
Comment at: include/clang/AST/Expr.h:440
+ ///
+ /// Likewise bitfields, we model gl-values referring to packed-fields as
+ /// an aspect of the value-kind type system.
----------------
aaron.ballman wrote:
> Like bitfields
> glvalues
"Likewise bitfields" -> "Like bitfields" still needs to be done.
Also, "packed-fields" -> "packed fields" throughout this comment.
================
Comment at: lib/AST/Decl.cpp:3523
+ return !isBitField() &&
+ (this->hasAttr<PackedAttr>() || getParent()->hasAttr<PackedAttr>()) &&
+ Context.getDeclAlign(this) <
----------------
rogfer01 wrote:
> rsmith wrote:
> > Does this properly handle anonymous struct/union members:
> >
> > struct A __attribute__((packed)) {
> > char a;
> > union { int b; };
> > } a;
> > int &r = a.b;
> This test (modulo swapping `A` and `__attribute__((packed))` positions)
> works. Do you think we may be missing some case here?
No, I think this is OK, so long as we have a unit test for it. I assume that
what happens here is that the implicit `a.<anonymous union object>` produces a
packed lvalue, and then the `.b` just preserves the packedness -- that is, the
anonymous union object is a packed field, but `b` is not?
================
Comment at: lib/Sema/SemaExprMember.cpp:1767-1768
if (!IsArrow) {
- if (BaseExpr->getObjectKind() == OK_Ordinary)
+ if (BaseExpr->getObjectKind() == OK_Ordinary ||
+ BaseExpr->getObjectKind() == OK_PackedField)
VK = BaseExpr->getValueKind();
----------------
I think the existing code here is checking for a condition that can't happen.
Do any tests fail if you replace this `if`/`else` block with just `VK =
BaseExpr->getValueKind();`?
================
Comment at: lib/Sema/SemaInit.cpp:4186
+ if (IsLValueRef && Initializer->refersToPackedField() &&
+ Initializer->getType()->getAsCXXRecordDecl()) {
----------------
rsmith wrote:
> rsmith wrote:
> > You don't need to special-case packed fields here. They just happen to be
> > the only non-addressable object kind we have right now that can be a class
> > type. If we had others, they should get this treatment too, so it seems
> > better to remove the check.
> Why are you treating lvalue references specially here? GCC doesn't seem to,
> and it's not obvious to me why rvalue references should get special treatment.
You don't seem to have addressed this comment: you can delete the
`refersToPackedField` check here; the check for a class type is sufficient.
https://reviews.llvm.org/D23325
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
