AaronBallman 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.
My opinion on this is evolving -- this morally is similar to all the padding warnings we have in that it's telling you "hey, this packing is suboptimal". Those warnings are all off-by-default because they're not identifying a logical mistake in your code; your code still *works* even though it may be possible to improve it. So I think leaving this as an off-by-default warning is reasonable. And it is a distinct diagnostic from `-Wpadded-bitfield` in that it's identifying new problematic patterns which may not be problematic on all platforms, so it should get its own warning group. Maybe that warning group sits under `-Wpadded-bitfield`, but we should give users a way to say "I want to know about bitfield padding issues, but not ones specific to how Visual Studio does things". https://github.com/llvm/llvm-project/pull/117428 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits