hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:2414
+ assert(PreferredAlign >= ABIAlign &&
+ "PreferredAlign is at least as large as ABIAlign.");
+ return PreferredAlign;
----------------
Minor nit: s/is/should be/;
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1207
+ if (!Base->Class->isEmpty())
+ HasNonEmptyBaseClass = true;
+
----------------
Just set `HandledFirstNonOverlappingEmptyField` to `true` with a comment before
the `if`:
By handling a base class that is not empty, we're handling the "first
(inherited) member".
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1799
if (D->isBitField()) {
+ if (FoundNonOverlappingEmptyFieldToHandle)
+ // For a union, the current field does not represent all "firsts".
----------------
Add a comment before the `if`:
```
// 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.
```
Given the rationale from the comment, move the subject `if` to immediately
after the determination of `FoundNonOverlappingEmptyFieldToHandle`. That way,
the setting of `HandledFirstNonOverlappingEmptyField` becomes less complicated
to track.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1832
const ArrayType* ATy = Context.getAsArrayType(D->getType());
FieldAlign = Context.getTypeAlignInChars(ATy->getElementType());
} else if (const ReferenceType *RT = D->getType()->getAs<ReferenceType>()) {
----------------
It seems that the code to set `AlignIsRequired` is missing from this path.
```
typedef double Dbl __attribute__((__aligned__(2)));
typedef Dbl DblArr[];
union U {
DblArr fam;
char x;
};
U u[2];
extern char x[sizeof(u)];
extern char x[4];
```
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1908
+ FoundNonOverlappingEmptyFieldToHandle))) {
+ HandledFirstNonOverlappingEmptyField = !IsUnion;
+
----------------
I think the `if` condition above is too complicated as a filter for setting
`HandledFirstNonOverlappingEmptyField`. I would prefer if we don't need to set
`HandledFirstNonOverlappingEmptyField` here. Please see my other comment about
`HandledFirstNonOverlappingEmptyField`.
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
