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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
