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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
hubert.reinterpretcast wrote:
> 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.
Thanks, that works too. I will address it in the next commit.


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

https://reviews.llvm.org/D79719