[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Eli Friedman via Phabricator via cfe-commits]

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