[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:1263
 
-  // The maximum field alignment overrides base align.
+  assert(!IsUnion && "Unions cannot have base classes.");
+  // AIX `power` alignment does not apply the preferred alignment for non-union
----------------
It seems this is a leftover copy of the code that has been moved above?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1809
+      Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  bool FoundFirstNonOverlappingEmptyField = false;
+  if (DefaultsToAIXPowerAlignment)
----------------
The rename I suggested in my previous round of review was in coordination with maintaining the value not just for AIX. Since we're only maintaining the value for AIX, I prefer the previous name (or `FoundFirstNonOverlappingEmptyFieldForAIX`).


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1810
+  bool FoundFirstNonOverlappingEmptyField = false;
+  if (DefaultsToAIXPowerAlignment)
+    if (!HandledFirstNonOverlappingEmptyField) {
----------------
Please merge the `if` conditions to reduce nesting:
```
  if (DefaultsToAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField) {
```


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1817
+      // We're going to handle the "first member" based on
+      // `FoundNonOverlappingEmptyFieldToHandle` during the current invocation
+      // of this function; record it as handled for future invocations.
----------------
Keep this reference to the variable up-to-date with its name.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1820
+      if (FoundFirstNonOverlappingEmptyField)
+        // For a union, the current field does not represent all "firsts".
+        HandledFirstNonOverlappingEmptyField = !IsUnion;
----------------
Change the condition of the `if` here to `!IsOverlappingEmptyField` and move the setting of `FoundFirstNonOverlappingEmptyField` to `true` into this `if`.

Move the previous comment and merge it with this one here:
> [ ... ] record it as handled for future invocations (except for unions, because the current field does not represent all "firsts").




================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1928
+      FoundFirstNonOverlappingEmptyField) {
+    auto upgradeAlignment = [&](const BuiltinType *BTy) {
+      if (BTy->getKind() == BuiltinType::Double ||
----------------
Sorry for not seeing this earlier (I only notice some things when I hide the inline comments). I think `performBuiltinTypeAlignmentUpgrade` would read better at the call site (and better capture the checking, which is based on the kind of built-in type, that is within the lambda).


================
Comment at: clang/test/Layout/aix-Wpacked.cpp:9
+
+// CHECK-NOT: warning: packed attribute is unnecessary for 'Q' [-Wpacked]
+// CHECK: warning: packed attribute is unnecessary for 'test2::C' [-Wpacked]
----------------
Clang diagnostics are normally checked using `-verify` (as opposed to `FileCheck`). To use it, I think this needs to be split into the "expecting no diagnostics" and the "expecting diagnostics" cases. As it is, I think the `CHECK-NOT` has a problem because it checks for plain `'Q'`.


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

https://reviews.llvm.org/D79719