[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D150226#4515783 <https://reviews.llvm.org/D150226#4515783>, @dblaikie wrote:

> In D150226#4498024 <https://reviews.llvm.org/D150226#4498024>, @aaron.ballman wrote:
>
>> In D150226#4461846 <https://reviews.llvm.org/D150226#4461846>, @efriedma wrote:
>>
>>> The fundamental problem here is the interaction with SFINAE; if we don't diagnose this as an error, we actually miscompile some cases.  Because of that, a flag wouldn't really be "turning off the warning"; it would be enabling a non-compliant language mode that has different rules for whether enum casts are legal.
>>>
>>> If it weren't for that, I don't think anyone would be in a hurry to turn the warning into a hard error.
>>
>> +1 to this. I'm worried about the miscompiles continuing in the wild. We did the best we could when landing the warning-as-error changes in D131307 <https://reviews.llvm.org/D131307> by telling users explicitly "This diagnostic is expected to turn into an error-only diagnostic in the next Clang release." in the release notes. Obviously, that's not ideal because very few folks read release notes, but we don't have any better mechanism for communicating these kinds of changes.
>>
>> What do folks think is a reasonable path forward here? Given that we're going to branch the Clang 17 release in a few weeks (AIUI), my suggestion is to kick the can down the road by landing these changes just after we branch. That way the changes land such that early adopters can test and see how disruptive we're being without time pressure or hassle of dealing with release branches. Do folks think that's a reasonable approach? (I'm open to other suggestions.)
>
> I'm not quite following if/what the objection is to removing the "ignore in system headers" for the warning for a release or two prior to making this a hard error? That seems like a fairly low-cost change (it does kick the can down the road a release or two before it becomes a hard error - but isn't worse for users, for the most part - if they were going to get a hard error from a system header before, at least now instead they'd get a warning or warning-defaulted-to-error instead, they could turn it off (but they had been warned that their system headers were going to cause them problems soon) and then it becomes a hard error a release or two later).

Thank you for bringing that up, sorry for not being more explicit -- by "these changes", I meant "whatever form we decide they should take" as opposed to "the patch as it is now."

In terms of removing the "ignore in system headers" flag, I'm not strongly opposed to it, but I do worry it will throw the baby out with the bathwater. The specific use case I'm worried about is: user has a system header with the problematic code, they run into this diagnostic and they can't address it themselves so they disable the warning for the project. The user then doesn't learn about the problems in their own (non-system header) code until it becomes a hard error later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226