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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
