[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#4422856 <https://reviews.llvm.org/D150226#4422856>, @dblaikie wrote:

> In D150226#4417539 <https://reviews.llvm.org/D150226#4417539>, @aaron.ballman wrote:
>
>> In D150226#4408980 <https://reviews.llvm.org/D150226#4408980>, @dblaikie wrote:
>>
>>> 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
>>
>> +1, I think that request is a reasonable idea to help maintainers of system headers.
>>
>> It sounds like we're in agreement that we should not enable `ShowInSystemHeaders` for this, but it's not clear whether we've got agreement yet that we can land this change right now. I think it's acceptable to kick the can down the road by another release or so if we feel we need to, but there is a hard stop to that at some point (some folks won't fix their code until they're forced to do so, and there's not much we can do about those cases).
>
> FWIW, I still think it might be reasonable to `ShowInSystemHeaders` before turning something into an unconditional/hard error - `ShowInSystemHeaders` is strictly less intrusive than a hard error, and more intrusive than the warning as-is, so seems like a reasonable part of transitioning to a feature removal. (warning -> default-error -> show in system headers -> hard error) I don't have enough of an investment in this area to suggest that this /must/ be the way such a transition is done, but I have a hard time seeing why it would be better to avoid that intermediate step.

My concern with `ShowInSystemHeaders` is that this seems like a bad user experience. If the system header triggers an error 1) some users aren't going to know how to fix that by downgrading the diagnostic, so that may cause them to go "Clang is buggy because <other compiler> accepts this fine" (not the end of the world, but frustrates both us and users). 2) the only recourse users have is to downgrade/disable the diagnostic (otherwise they'd have to change system header code), which they may likely do with a command line flag rather than something more targeted like diagnostic pragmas around the include, which increases the risk of users not seeing the issues in code they can control.

That said, I see your logic and kind of agree with it. I just worry if it's going to cause more harm than good (and I don't know of a particularly good way to try to find out aside from "try it and see").


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

https://reviews.llvm.org/D150226