hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1064
setSize(getSize() + PtrWidth);
setDataSize(getSize());
}
----------------
I would suggest setting `HandledFirstNonOverlappingEmptyField` to `true` here
with an assertion that the current type is not a union.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+ bool FoundFirstNonOverlappingEmptyFieldToHandle =
+ DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+ !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
The condition is still more complex than I think it should be.
If we have found a "first" other-than-overlapping-empty-field, then we should
set `HandledFirstNonOverlappingEmptyField` to `true` for non-union cases.
If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for
`FieldOffset == CharUnits::Zero()` to be true, then I think the correction
would be to set `HandledFirstNonOverlappingEmptyField` in more places.
I would like to remove the check on `FieldOffset == CharUnits::Zero()` from
here and instead have an assertion that `!HandledFirstNonOverlappingEmptyField`
implies `FieldOffset == CharUnits::Zero()`.
Also, since we're managing `HandledFirstNonOverlappingEmptyField` in non-AIX
cases, we should remove the `DefaultsToAIXPowerAlignment` condition for what is
currently named `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting uses of
it as necessary) and rename `FoundFirstNonOverlappingEmptyFieldToHandle` to
`FoundFirstNonOverlappingEmptyField`.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1832
EffectiveFieldSize = FieldSize = CharUnits::Zero();
const ArrayType* ATy = Context.getAsArrayType(D->getType());
+ TypeInfo TI = Context.getTypeInfo(D->getType());
----------------
`ATy` seems to be an unused variable now.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+ TypeInfo TI = Context.getTypeInfo(D->getType());
+ FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+ AlignIsRequired = TI.AlignIsRequired;
----------------
I guess this works (we have a test for it), but the previous code made a point
to use the element type and not the array type (and the comment above says we
can't directly query `getTypeInfo` with the array type). @Xiangling_L, can you
confirm if the comment is out-of-date and update it?
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1909
+ if (DefaultsToAIXPowerAlignment && !AlignIsRequired &&
+ (IsUnion || FoundFirstNonOverlappingEmptyFieldToHandle)) {
+ auto upgradeAlignment = [&](const BuiltinType *BTy) {
----------------
It should now be the case that `FoundFirstNonOverlappingEmptyFieldToHandle` is
`true` for all union members that are not empty, meaning that the `IsUnion`
part of the check only serves to admit attempts to handle types that are empty
(and thus does not have subobjects that would induce an alignment upgrade).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79719/new/
https://reviews.llvm.org/D79719
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
