sfertile added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:16447
+
+ bool AIXBitfieldViolation = false;
+ if (const BuiltinType *BTy = FieldTy.getTypePtr()->getAs<BuiltinType>()) {
----------------
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.
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
