Using std=c++2a is now equivalent to std=c++20 (if both flags are supported by the compiler).
Features use non-fallback standard names (c++20 instead of c++2a).
E.g. using --param=std=c++2a will detect feature c++20.
Note: you still cannot pass --param=std=c++23 if the compiler doesn't support -std=c++23.
Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
11–15 | I have two weak rationales here:
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 like this patch a lot. This makes it a lot easier to switch to newer C++ versions shortly after their release without the need to wait for the compilers to update.
Please update or rebase the patch to trigger the CI.
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
11–15 | 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. |
- Don't use c++23.
- Rebase.
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
11–15 | @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. |
@ldionne, I'd like to hear you input on the changes in params.py. At least just to check that I haven't done anything stupid (given my lack of knowledge of lit and related stuff).
Apart from that, I start thinking of the plan for adding new standard, if anyone sees other points, please LMK.
Example: adding c++2c, c++23 (before: c++2b) released:
- In libcxx/utils/libcxx/test/params.py:
- Change c++2b to c++23, add c++2c to _allStandards.
- Add 'c++23': 'c++2b' to _standardFallback dictionary.
- Change c++2b to c++23 and add c++2c to std_dialects list in libcxx/utils/generate_feature_test_macro_components.py.
- Add C++2c CI job to libcxx/utils/ci/buildkite-pipeline.yml.
- Add generic-cxx2c to libcxx/utils/ci/run-buildbot.
- Add libcxx/cmake/caches/Generic-cxx2c.cmake CMake cache file.
- [Optionally] Add new feature-test macros libcxx/utils/generate_feature_test_macro_components.py.
- In libcxx/include/__config, bump _LIBCPP_STD_VER for C++2b and add a new value for C++2c.
- In libcxx/test/support/test_macros.h, set TEST_STD_VER to 23 for C++2b and to 99 for C++2c.
- In libcxx/test/support/msvc_stdlib_force_include.h, bump TEST_STD_VER for MSVC.
- In libcxx/CMakeLists.txt, use CXX_STANDARD 23 (or higher?).
Can anyone help me with the CI failure (Runtimes build)?
https://buildkite.com/llvm-project/libcxx-ci/builds/2347#91e9025e-e17d-41eb-a9ba-3bd5fd28c086
(TBH, I'm looking for a confirmation that it's unrelated to this patch, as I've never seen a failure in this test, test_demangle.pass.cpp, and the message libc++abi: terminating is not very informative)
I'm seeing that in my recent CI runs too - so it's most probably unrelated to your patch, but no idea what triggered it...
OK, right, it seems that it's a non-libc++ change that provoked this failure (Runtimes build uses just-built ToT clang).
FYI.
There are few possible culprits out there, some commit between today (6th April 2021) 8:00 (green https://buildkite.com/llvm-project/libcxx-ci/builds/2342#7a94194a-b986-449a-8c6c-4952a1ec8763, commit b7ef804807855e607da3eba221c1fc59e27f778e) and 10:00 UTC (this patch, red: https://buildkite.com/llvm-project/libcxx-ci/builds/2347#91e9025e-e17d-41eb-a9ba-3bd5fd28c086, based on main's commit f8f4d8f87ba4c1fbb18a4e7f4a5ea03a8b8ec061).
Looks acceptable to me, but I think my comment on sized_delete11.pass.cpp is worth addressing.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp | ||
---|---|---|
14 | 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? | |
libcxx/utils/libcxx/test/params.py | ||
11–15 | Yes, thank you! Specifically https://reviews.llvm.org/D97365#2604643 was the comment I was looking for. | |
14 | 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. |
The patch looks good to me, but I think we can simplify the implementation in params.py a bit IMO. I left some comments to that effect.
I also agree with the list of things to do when a new standard comes out - do you think it would be possible to document it somewhere? Maybe in Contributing.rst?
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
11–14 | ||
45–53 | ||
58–62 |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
58–62 | Also, in actions, I don't have cfg, only std... |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
58–62 | WDYT about: AddFeature(next((s for s, f in reversed(_allStandards) if f == std), std)), ? |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
58–62 | Yeah, it seems I really botched my code change suggestion :-). Basically anything that works is fine by me, but I found the logic with multiple variables at the beginning of the file to be hard to follow. |
libcxx/utils/libcxx/test/params.py | ||
---|---|---|
11–14 | I've kept c++1z as I sometimes use oldish gcc (e.g. cross-compiling for AVR). Hope that doesn't bother. | |
58 | We need to have both keys and values here to allow fallbacks. I'll land it with this suggestion but I don't want to trigger yet another CI only for this (given that the choices is visible only when the user provides a bad argument to --param=std, it isn't really tested anyway). | |
60 | The default needs to be [1] because we don't want to use -std=c++20 on the compiler that supports only -std=c++2a. |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp | ||
---|---|---|
14 | Nice! I did not know || would work here. :) | |
libcxx/utils/libcxx/test/params.py | ||
11–15 | I like this. Is it worth adding c++0x, c++1y as fallbacks for those two? I assume the logic of leaving them out is simply that we don't officially support any compiler old enough to lack a c++14 mode. But it might be useful-to-someone and/or encyclopedic to keep them in the script, I dunno. | |
58–62 | I think all we're actually doing here is saying, "If the user gave us c++2a, turn it into c++20." Right? So that would be, like, def what_were_doing(flag): if flag in _allStandards.values(): AddFeature(next(s for s, f in _allStandards.items() if f == flag)) else: AddFeature(flag) which is like... exactly what you wrote, except without the reversed, right? The reversed would make a difference only if _allStandards.items() were in some relevant order to begin with. I'm not thrilled by the readability of this expression, but I have no concrete improvements to suggest. So I think I'm just asking to remove the reversed(...). |
Sorry it took me so long to understand what this patch was doing, however IMO we don't want to allow people to pass --param std=c++2a to Lit. We want to use --param std=c++20, and then have that query the compiler for either -std=c++20 or -std=c++2a if that's the only thing it supports. I think doing that is simpler, and it leaves the "legacy" -std=XXX parameter names to be handled as implementation details of the test suite without making them prominent when using the test suite.
@ldionne wrote:
IMO we don't want to allow people to pass --param std=c++2a to Lit. We want to use --param std=c++20
Unfortunately that doesn't scale. Today we force people to use --param std=c++2a (because this patch hasn't been committed); and we also force people to use --param std=c++2b (because C++2b hasn't got an official number yet). We don't want to force everyone to have a "flag day" where they all simultaneously get forced over from std=c++2a to std=c++20; we want them to be able to adjust their scripts and stuff on their own time. So, since we need to support std=c++2a as a synonym for std=c++20 here anyway at the compiler level, I think it's quite correct to just reuse that list of synonyms for argument parsing as well. Which is what this patch does.
TLDR: This morning I ran ./bin/llvm-lit --param std=c++2a ../libcxx/test/. I want to run that same command tomorrow morning.
I disagree - this is just the interface for running Lit from the command line, I think it's entirely fine if there's a "flag" day, and even if it's not official. There are some things that we need to create engineering solutions for in order not to break people, but I don't think that's one. I think pretty much the only places where there's --param std=c++2a is going to be in the CI scripts (which can be updated in lockstep with this patch), and in the libc++ developers' shell command history or local helper functions. I think it's fine if they just need to change it to --param std=c++20 at some point.
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.