[PATCH] D23325: [WIP] Binding of references to packed fields

[Richard Smith via cfe-commits]

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