aaron.ballman added a comment. In D150226#4516337 <https://reviews.llvm.org/D150226#4516337>, @dblaikie wrote:
> In D150226#4515843 <https://reviews.llvm.org/D150226#4515843>, @aaron.ballman > wrote: > >> 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." > > Ah, OK. > >> 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. > > OK, I think we're still talking past each other/making different comparisons. > I think you're comparing "make this a warning in system headers" to "not > doing anything" (so, yes, it causes people to ignore the warning even in > their own code when if they had done nothing/we had done nothing to force > them, they would've kept getting the warning in their code) > Whereas I'm comparing "make this a warning in system headers" to "making this > a hard error" (in which case a user for which it would've been a hard error > and they'd have had no choice but to fix their system headers (might be > hard/impossible) would have some relief valve for a time/some notice that > this would be a problem in the future when it becomes a hard error) > > If we picture a timeline: > Time 1) Code is valid/no problem > Time 2) Warning added (not in system headers) > Time 3) Warning becomes error by default (but could be disabled by a user) > (still not in system headers) > Time 4) Becomes hard error > > And if someone has a system header with a violation, they won't know about it > until (4) and they won't be able to do much/anything about it. > > I'm suggesting making the process longer. > > Time 4) default-error warning becomes enabled in system headers > > Users who, in the first timeline, would be stuck - at least have a release > valve. I think they are better off than they were in the first timeline - > even though they lose warning coverage over their own code (which is bad), at > least they can still build their project and know they have problems in their > system headers they should report/etc and try to have addressed. > > then Time 5) make it a hard error. > > So it takes a bit longer to get to (5), and users who would be OK in the > original timeline are no worse off - they had there system headers fixed by > (4) and don't need to disable the warning. But users who would've had a bad > time at (4) have a less bad time/some chance of working through the bad time. Thank you for the detailed explanation, that's really helpful! I think we were approaching this from somewhat different angles, but I now better understand what you mean. The tradeoff boils down to a question of which is less bad for users: losing warning coverage in their own code or finding out they have to get new system headers (or modify the existing ones, but no easy switch to address the issue). I think getting new system headers is harder for projects to deal with, so I think I'm now in agreement with you that we should enable the warning as an error in system headers. 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
