hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:2416
- // Double and long long should be naturally aligned if possible.
+ // Double, long double (only when the target supports AIX power alignment)
and
+ // long long should be naturally aligned if possible.
----------------
I suggest to clarify the binding of the parenthetical and also to make the
context that the required alignment is lower than the natural alignment more
explicit:
```
// Double (and, for targets supporting AIX `power` alignment, long double) and
// long long should be naturally aligned (despite requiring less alignment) if
// possible.
```
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1210
+ auto getBaseOrPreferredAlign = [&](CharUnits UnpackedAlign) {
+ return (Packed && ((Context.getLangOpts().getClangABICompat() <=
----------------
The lambda is currently being named for what is produced based on the identity
of what gets passed to it and not named for what it does.
This should be named `getPackedBaseAlignFromUnpacked` or similar.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1212
+ return (Packed && ((Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver6) ||
+ Context.getTargetInfo().getTriple().isPS4()))
----------------
I suggest not applying "ABI compat" with Clang <= 6 on AIX. There was is no
Clang <= 6 with AIX support.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1218
+
// Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
// Per GCC's documentation, it only applies to non-static data members.
----------------
This comment belongs inside the lambda.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
+ // Do not use AIX special alignment if current base is not the first member
or
+ // the struct is not a union.
----------------
Suggestion:
```
// AIX `power` alignment does not apply the preferred alignment for non-union
// classes if the source of the alignment (the current base in this context)
// follows introduction of the first member with allocated space.
```
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1769
+
+ bool FoundNonOverlappingEmptyField = false;
+ bool SupportsAIXPowerAlignment =
----------------
The name is wrong for the value (the value corresponding to this name is
`!IsOverlappingEmptyField`). I would suggest something like
`FoundNonOverlappingEmptyFieldToHandle`.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1772
+ Context.getTargetInfo().defaultsToAIXPowerAlignment();
+ if (SupportsAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField &&
+ !IsOverlappingEmptyField)
----------------
Move the definition of the variable here and just use the `if` condition as the
initializer.
Repository:
rG LLVM Github Monorepo
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
