[clang] Add an off-by-default warning to complain about MSVC bitfield padding (PR #117428)
Oliver Hunt via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 4 16:35:23 PST 2024
ojhunt wrote:
> > > So I take it we decided not to enable it by default in `-fms-compatibility` mode then?
> >
> >
> > I don't believe it is appropriate to do so. The intent of this warning is to indicate MSVC compatibility issues when building in non-ms-compatibility modes. The existing padding warnings already trigger for these layouts in the relevant ms compatibility modes
>
> We don't typically add new off-by-default warnings though; users don't enable them often enough to be worth the costs. My thought process is: if we enable this diagnostic by default in `-fms-compatibility` mode as telling the user their bit-fields aren't packing, then it's on by default for some configurations so it meets our bar for inclusion, and it helps us directly because we have Windows precommit (and post-commit) CI coverage for building Clang and LLVM. Users on other platforms can opt-in to the diagnostic if they want the diagnostics for compatibility reasons.
My concerns with attaching it to `-fms-compatibility` is that projects using that and that care about padding already have the padding/packing warnings enabled. For every other user we will be warning them that the padding matches the abi they're targeting. The problem I'm wanting to address is not "I am targeting the ms abi" it is "I am not targeting ms abi but want to be told about packing/padding that would be different on ms platforms".
The intention would be to then enable the warning (maybe with a `-Werror=...` once the build is clean :D ) for all llvm projects, not just the windows targets - maybe with some evangelism so larger projects that have similar issues can be made aware of the flag. Then we can start migrating away from unending unsigned bit-fields and adopt enum bit-fields
>
> If we want to leave it off by default, then I wonder if we want to roll the functionality into `-Wpadded-bitfield` which already exists and is off by default.
The problem with this is that that people who do not care about windows/ms abi will then be getting what to them are spurious warnings.
https://github.com/llvm/llvm-project/pull/117428
More information about the cfe-commits
mailing list