[PATCH] D132877: Downgrade the UAX31 diagnostic to a warning which defaults to an error

Mon Aug 29 14:02:41 PDT 2022

aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

In D132877#3756458 <https://reviews.llvm.org/D132877#3756458>, @cor3ntin wrote:

> I am strongly against this change. If we do want to do this, we need to restore the original tables from c++11 and check against them not to extend the set of characters to something much wider than was supported before c++23. It should be fairly easy to extract those tables from the git history. (Sorry for the very short comment, I'm currently traveling). A warning for non characters, controls, pua characters and everything not previously accepted isn't sufficient. A warning for things in the old set but not 23 is fine as long as, as other have said, we have a depreciation period.

Thanks for this feedback! Ugh, I didn't realize we were going to need to keep around both sets of tables, that potentially adds more expense to this option than I originally realized, depending on how much extra binary size the tables add.

In D132877#3756475 <https://reviews.llvm.org/D132877#3756475>, @efriedma wrote:

> I'm not really a fan of the argument of "this was accepted as a DR, therefore new versions of clang have to reject code that old versions accepted".  Regardless of what the C++ committee does, our commitment to our users is to ensure that code written against "-std=c++11" continues to compile in newer versions of clang.

This is the status quo in Clang. When one of the committees says something is a DR, the expectation users have is that the feature was always specified to behave in the fixed way. This case is a bit special though because the old rules were approximately 30+ years old and were "use what you like so long as it's not punctuation, yolo" and the new rules are more restrictive. I think our commitment  is a bit weaker than "ensure code continues to compile." We've never promised we wouldn't fix bugs against older standards, and fixing bugs can break otherwise working code. (And WG21 considers this a bug, hence the DR designation.) However, you're definitely right that we're not *obligated* to make the change in older language modes just because the committee said it's a DR. We are obligated to implement the functionality in a conforming manner in newer language modes though.

It's worth noting that the code which is broken by this is largely within one domain (use of math symbols), so the number of users impacted is believed to be relatively small (small enough that we were considering closing the issue as Won't Fix instead of making this change). The reason I decided to propose a relaxation here was to give the handful of impacted users an easier upgrade path.

> I don't want to relitigate the whole discussion from the bug, though; just wanted to make sure you considered the possibilities. If you think this is best, I won't object.

Your opinions are very much appreciated on the topic, it's not one with a slam dunk answer (at least that we've found yet).

In D132877#3756415 <https://reviews.llvm.org/D132877#3756415>, @thieta wrote:

> Hmm. 15.0.0 is just a week away - I am not planning any more RCs unless we hit something critical. What's the risk of taking this specific change at this point? Would it make more sense to wait for 15.0.1? (I am guessing it's better if it goes into 15.0.0 or not in 15.x at all).
>
> I am just pushing back because I don't want to introduce to much risk this late in the cycle - but I will defer to you guys since you are the domain experts.

Given the feedback from Corentin above, I think the window for getting this into Clang 15 is effectively closed. It's not critical (it doesn't fix a crash or a regression); I was mostly hoping it would make it in 15.0 because it doesn't seem appropriate to introduce a new warning flag in a dot release (that would seem to make build systems harder to work with in practice).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132877