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