[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double
Eli Friedman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 8 19:23:23 PDT 2020
efriedma added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:666
+ FirstNonOverlappingEmptyFieldHandled
+ } FirstNonOverlappingEmptyFieldStatus;
+
----------------
Xiangling_L wrote:
> efriedma wrote:
> > Instead of specifically tracking whether you've found an OverlappingEmpty field, could you just have something like "bool FoundNonOverlappingEmptyField = false;", and set it to true when you handle a field that isn't OverlappingEmpty? I don't think we need to specifically track whether we've found an OverlappingEmpty field.
> I think you are right. We do not need to specifically track whether we've found an OverlappingEmpty field. But I think we do need an enum to track if the first non-OverlappingEmptyField is handled or not.
>
> Or the following case is problematic:
>
>
> ```
> struct A {
> int : 0;
> double d;
> };
>
> ```
> The `double d` will mistakenly match `FieldOffset == CharUnits::Zero() && D->getFieldIndex() != 0 && !IsOverlappingEmptyField && FoundNonOverlappingEmptyField `, which we shouldn't because it is not the first member of the struct.
>
>
>
Okay, so now there are three states. But the FirstNonOverlappingEmptyFieldFound is transient: on exit from ItaniumRecordLayoutBuilder::LayoutField, FirstNonOverlappingEmptyFieldStatus never contains FirstNonOverlappingEmptyFieldFound. I think it would be more clear to use a local variable for that.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1884
+ if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+ ((D->getFieldIndex() == 0 && !IsOverlappingEmptyField) || IsUnion ||
+ (D->getFieldIndex() != 0 && FirstNonOverlappingEmptyFieldStatus ==
----------------
I don't think you need to distinguish the `D->getFieldIndex() == 0` case from the `D->getFieldIndex() != 0` case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79719/new/
https://reviews.llvm.org/D79719
More information about the cfe-commits
mailing list