[libcxx-commits] [PATCH] D99789: [libc++] Add fallback standard flags and normalize corresponding feature.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Looks acceptable to me, but I think my comment on sized_delete11.pass.cpp is worth addressing.



================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp:14
 
-// UNSUPPORTED: c++14, c++17, c++2a
+// UNSUPPORTED: c++14, c++17, c++20
 // UNSUPPORTED: sanitizer-new-delete
----------------
Here and above, it looks like the intent is "`REQUIRES: c++03 OR c++11`". I don't know if there's any way to express that in llvm-lit. However, I predict that these tests fail in `c++2b` mode; am I right?
It might be better to duplicate these files into `sized_delete11.pass.cpp` and `sized_delete03.pass.cpp`, add an appropriate `REQUIRES:` to each of them, and then never touch them again.


================
Comment at: libcxx/utils/libcxx/test/params.py:14
+_standardFallbackReverse = dict((v, k) for k, v in _standardFallback.items())
+_allStandardsAndFallbacks = _allStandards + [fallback for s, fallback in _standardFallback.items()]
 _warningFlags = [
----------------
I believe this line could be
```
_allStandardsAndFallbacks = _allStandards + list(_standardFallback.values())
```

Orthogonally, how about `s/_standardFallbackReverse/_undoStandardFallback/`? I think that would make line 62 read a little clearer.


================
Comment at: libcxx/utils/libcxx/test/params.py:11-12
 
-_allStandards = ['c++03', 'c++11', 'c++14', 'c++17', 'c++2a', 'c++2b']
+_allStandards = ['c++03', 'c++11', 'c++14', 'c++17', 'c++20', 'c++23']
+_standardFallback = {'c++11': 'c++0x', 'c++14': 'c++1y', 'c++17': 'c++1z', 'c++20': 'c++2a', 'c++23': 'c++2b'}
+_standardFallbackReverse = dict((v, k) for k, v in _standardFallback.items())
----------------
curdeius wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > I have two weak rationales here:
> > > - Technically, it's not "C++23" until it's released. I don't want to get into a situation where we have to //remove// the string `c++23` from this codepath. So we should leave it as `c++2b` until 2023.
> > > - I'd like to see us develop a "deploy plan" for moving from one standardization-epoch to the next, which we execute atomically every 3 years. I described my ideal here: [SADLY I CANNOT LOCATE THIS REVIEW ANYMORE maybe @Mordante remembers our discussion and can dig it up?] Therefore I'd like this patch to get us into the "C++20 has been released, C++2b has not yet been released" state. Three years from now, we can atomically move into the "C++23 has been released, C++2c has not yet been released" state. I don't want us to ever be in some halfway state like the "C++23 has been released sorta not really" state described in this patch right now.
> > > 
> > > D93383 was related. D65043 was loosely related.
> > > 
> > > I //think// (but am not 100% sure) that all I'm asking for here is to replace `'c++23'` with `'c++2b'` in the one place it appears. Everything else can stay the same, right?
> > I recall the discussion, but also can't find it anymore. I'm also slightly in favour to use `c++2b` instead of `c++23`. I get the impression the members in the committee feel more comfortable with the 3 year plan so I would be very surprised when `c++23` becomes `c++24`. Still I think using `c++2b` safer and give a nice statement the standard isn't released.
> @Quuxplusone, have you had in mind the discussion from https://reviews.llvm.org/D97365#2603777, https://reviews.llvm.org/D97365#2604643?
> 
> Also, https://reviews.llvm.org/D93383 mentioned this a bit too.
Yes, thank you! Specifically https://reviews.llvm.org/D97365#2604643 was the comment I was looking for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99789