dblaikie added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150226/new/ https://reviews.llvm.org/D150226 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
