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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > 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`.
> > Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX cases, we should remove the DefaultsToAIXPowerAlignment condition for what is currently named FoundFirstNonOverlappingEmptyFieldToHandle 
> 
> I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` condition and bother with maintaining correct status of `HandledFirstNonOverlappingEmptyField` for other targets.
> 
> We are actually claiming `HandledFirstNonOverlappingEmptyField` is an auxiliary flag used for AIX only in its definition comments.
> 
> Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in non-AIX cases, I noticed that we have to set this flag to `true` somewhere for objective-C++ cases. 
Okay, the other option I'm open is setting `HandledFirstNonOverlappingEmptyField` to `true` up front when not dealing with AIX `power` alignment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79719/new/

https://reviews.llvm.org/D79719