[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
       !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 
----------------
It seems a little wasteful and error-prone that we're now computing the actual alignment, the alignment if the field were not packed, and the alignment if the field were packed. Is there any way we can reduce this down to computing just the alignment if the field were packed plus the alignment if the field were not packed, then picking one of those two as the actual field alignment? Or does that end up being messier?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2140-2141
+
+  if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+    Diag(D->getLocation(), diag::warn_unpacked_field) << D;
 }
----------------
Hm. Doing this from here will mean we only warn if we actually compute the layout of the class. But I suppose that's the same as what we do a few lines above, and for other `-Wpacked` warnings, and it seems necessary if we want to suppress the warning if packing wouldn't have made any difference anyway.


================
Comment at: clang/test/CodeGenCXX/warn-padded-packed.cpp:157
+  char c2;
+  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is non-POD}}
+} __attribute__((packed));
----------------
Maybe consider adding another test showing that we don't warn if packing would have made no difference anyway (for example if the alignment of the POD struct was already 1).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511