hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1778
+ if (FoundNonOverlappingEmptyField)
+ HandledFirstNonOverlappingEmptyField = true;
+
----------------
See my other comment for rationale on why
`HandledFirstNonOverlappingEmptyField` should be set to `!IsUnion` instead of
`true`.
A comment would be appropriate here:
```
if (FoundNonOverlappingEmptyField) {
// For a union, the current field does not represent all "firsts".
HandledFirstNonOverlappingEmptyField = !IsUnion;
}
```
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1879
+ (IsUnion || FoundNonOverlappingEmptyField)) {
+ HandledFirstNonOverlappingEmptyField = true;
+
----------------
`IsOverlappingEmptyField` is still in play (as seen in further checks below). I
do not believe it is appropriate to set `HandledFirstNonOverlappingEmptyField`
to `true` in that case (possible because `IsUnion` overrides what is currently
called `FoundNonOverlappingEmptyField`).
There are a few ways to address this. For example,
`HandledFirstNonOverlappingEmptyField` could be set to `!IsUnion` instead for
to `true`. This is semantically sound, because
`HandledFirstNonOverlappingEmptyField` is meant to indicate that there would
not be further "firsts" to consider (and that is not true for unions).
I believe the code should not require user to convince themselves that
non-sequitur logic washes away, so I would like a fix for this although it
introduces no functional change.
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
