[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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4408563 <https://reviews.llvm.org/D150226#4408563>, @erichkeane wrote:

> In D150226#4408381 <https://reviews.llvm.org/D150226#4408381>, @aaron.ballman wrote:
>
>> In D150226#4404808 <https://reviews.llvm.org/D150226#4404808>, @rupprecht wrote:
>>
>>> I suppose including this warning in `ShowInSystemHeader` with your diff above would be a good first step anyway, right? If we're about to make it a hard error anyway, then making it a `ShowInSystemHeader` error first would ease that transition.
>>
>> Yes and no.
>>
>> If we're going to turn something into a hard error, letting folks implementing system headers know about it is important, so from that perspective, it makes sense to enable the diagnostic in system headers. However, *users* have no choice in their system headers (oftentimes) which makes the diagnostic unactionable for them as they're not going to (and shouldn't have to) modify system headers, so from that perspective, enabling the diagnostic in a system header introduces friction.
>>
>> If this diagnostic is showing up in system headers, I think we would likely need to consider adding a compatibility hack to allow Clang to still consume that system header while erroring when outside of that system header (we've done this before to keep STL implementations working, for example). Between this need and the friction it causes users to have an unactionable diagnostic, I think we probably should not enable `ShowInSystemHeader`.
>
> It seems to me that if our concern is breaking system headers, we need to do that with better testing.  Some sort of 'diagnostic group' for "This is going to become an error *SOON*" mixed with us/vendors running that on platforms we consider significant enough to not break.  But just diagnosing on arbitrary users with no choice on how to fix the headers doesn't seem appropriate.

I think that's the request here: https://github.com/llvm/llvm-project/issues/63180 - some way, initially, to opt-in to such a diagnostic group or mechanism (like "enable this warning in system headers") to assess how big the migration effort is/report back on whether it's necessary to implement a compatibility hack or whether things will be able to be cleaned up. (such a thing could be opt-in at first, vendors etc could use that to assess the fallout - if the answer is "we think we can clean this up" and folks plan some timeline, they reckon they're ready, then you switch to that feature being enabled by default (because we think we can make it an error - but there's always surprises, so shipping it as an error-by-default-but-with-a-way-to-opt-out is better than shipping it as a hard error that catches users by surprise) - then if there's no major fallout, remove the ability to opt-out)


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

https://reviews.llvm.org/D150226