Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:16447
+
+ bool AIXBitfieldViolation = false;
+ if (const BuiltinType *BTy = FieldTy.getTypePtr()->getAs<BuiltinType>()) {
----------------
sfertile wrote:
> Xiangling_L wrote:
> > sfertile wrote:
> > > Can this change can be split out into its own patch? If it can i would
> > > suggest doing so.
> > I was expecting our buildbot can pick up all bitfield related changes at
> > one time. Also if we split this out, that means we either need to wait for
> > this second part to land first or need to add more LIT to oversized long
> > long to the first part, which then needs to be removed whenever this second
> > part land. It seems we are complicating the patch. Can you give me your
> > rationale about why we want to split out this part?
> > I was expecting our buildbot can pick up all bitfield related changes at
> > one time.
> IIUC `clang/test/Layout/aix-oversized-bitfield.cpp` works with just this
> change and isn't dependent on D87702. Its disjoint from the other changes in
> this patch, and packaging it into a commit with unrelated changes even if
> they are on the same theme is not beneficial. Its better to have those run
> through the build bot (or be bisectable) as distinct changes.
>
> > Also if we split this out, that means we either need to wait for this
> > second part to land first or need to add more LIT to oversized long long to
> > the first part, which then needs to be removed whenever this second part
> > land. It seems we are complicating the patch.
>
> I don't understand why it would need to wait or require extra testing to be
> added. Its a diagnostic and your lit test shows the error for 32-bit (where
> we want it emitted) and expected layout for 64-bit. The whole point of
> splitting it out is that its simple,does exactly one thing, is testable on
> its own, and we don't need the context of the other changes packaged with it
> to properly review it. I am asking to split it out because I see it as making
> this easier to review and commit.
Sure, I will split this patch into two as you suggested. By `either need to
wait for this second part to land first or need to add more LIT `, I thought we
would like to also add test coverage and later remove it for oversize bitfield.
Since `StorageUnitSize > 32 &&
Context.getTargetInfo().getTriple().isArch32Bit()` does affect how oversize
bitfield get laid out on AIX. But I guess it's more convenient to just split
this patch as you suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87029/new/
https://reviews.llvm.org/D87029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
