dblaikie marked an inline comment as done.
dblaikie added inline comments.
================
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;
----------------
rsmith wrote:
> 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?
I had a go at that refactor - we can't pull the `FieldPacked` computation lower
(it'd be great if it could move down to after the packed/unpacked computation,
so it was clear that those values were computed independently of the
`FieldPacked` value, and that `FieldPacked` just determined which one to pick)
because of the `alignedAttrCanDecreaseAIXAlignment`, by the looks of it.
And also the AIX alignment stuff seems to do some weird things around the
preferred alignment that caused the carefully constructed 3 `if`s below
(`!FieldPacked`, `DefaultsToAIXPowerAlignment`, and `FieldPacked`) which I
spent more time than I'd like to admit figuring out why anything else/less/more
streamlined was inadequate.
But I also don't understand why `DefaultsToAIXAlignment` causes the `AlignTo`
value to be the `PreferredAlign`, but the `FieldAlign` stays as it is? (like
why doesn't `DefaultsToAIXPowerAlignment` cause `FieldAlign` to /be/
`PreferredAlign` - I think that'd simplify things, but tests (maybe the tests
are incorrect/) seemed to break when I tried that) - I would've thought not
doing that (as the code currently doesn't) would cause problems for the
`UnadjustedAlignment`, `UpdateAlignment`, and `warn_unaligned_access` issues
later on that depend on `FieldAlign`, but feel like they should probably depend
on the alignment that actually got used (the `PreferredAlign`) instead? It's
pretty confusing to me, so... yeah.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2140-2141
+
+ if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+ Diag(D->getLocation(), diag::warn_unpacked_field) << D;
}
----------------
rsmith wrote:
> 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.
Yeah, the test has that workaround `f` function that references all the types
for that reason - but could be nice to avoid that, though would mean moving all
this layout stuff somewhere else/more reusable, I guess? Probably out of scope
for this patch, but I'm open to ideas if it's worth addressing as a follow-up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118511/new/
https://reviews.llvm.org/D118511
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
