[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:1778
+    if (FoundNonOverlappingEmptyField)
+      HandledFirstNonOverlappingEmptyField = true;
+
----------------
See my other comment for rationale on why `HandledFirstNonOverlappingEmptyField` should be set to `!IsUnion` instead of `true`.

A comment would be appropriate here:
```
    if (FoundNonOverlappingEmptyField) {
      // For a union, the current field does not represent all "firsts".
      HandledFirstNonOverlappingEmptyField = !IsUnion;
    }
```


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1879
+      (IsUnion || FoundNonOverlappingEmptyField)) {
+    HandledFirstNonOverlappingEmptyField = true;
+
----------------
`IsOverlappingEmptyField` is still in play (as seen in further checks below). I do not believe it is appropriate to set `HandledFirstNonOverlappingEmptyField` to `true` in that case (possible because `IsUnion` overrides what is currently called `FoundNonOverlappingEmptyField`).

There are a few ways to address this. For example, `HandledFirstNonOverlappingEmptyField` could be set to `!IsUnion` instead for to `true`. This is semantically sound, because `HandledFirstNonOverlappingEmptyField` is meant to indicate that there would not be further "firsts" to consider (and that is not true for unions).

I believe the code should not require user to convince themselves that non-sequitur logic washes away, so I would like a fix for this although it introduces no functional change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719