[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double
Hubert Tong via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 8 18:58:55 PDT 2020
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
More information about the cfe-commits
mailing list