[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:1246
+
+    // AIX `power` alignment does not apply the preferred alignment for
+    // non-union classes if the source of the alignment (the current base in
----------------
Move the comment to above the previous `if` and make the following `if` the `else` of the previous `if`.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
Xiangling_L wrote:
> 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.
I'm not seeing the change for https://reviews.llvm.org/D79719?id=276143#inline-767942?


================
Comment at: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp:19
+  char x alignas(4)[8];
+} ;
+
----------------
Minor nit: Remove the space before the semicolon.


================
Comment at: clang/test/Layout/aix-Wpacked-no-diagnostics.cpp:15
+
+int a = sizeof(QQ);
----------------
Is there a reason to drop the `FileCheck` checking for the layout?


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

https://reviews.llvm.org/D79719