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

Reply via email to