-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement P3016R6 Resolve Inconsistencies In begin/end For valarray And Braced Initializer Lists #5847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@microsoft-github-policy-service agree |
|
Sections 4.1, most of 4.7 and 4.8 of the paper prescribe changes this STL already has (some Section 4.9 rewords the effects of some standard library member functions (mostly members of |
|
There are two failing tests (copied from CI):
The test is compiled in C++14 mode. The removed non-member overload It appears that we either can't implement this change unconditionally, or we have to drop |
|
For Dev11_1074023_constexpr, we can simply make that line of the test conditional on For libc++, we can use
Under the As If Rule, we aren't required to, and I don't see the need to change this. |
Only check that begin(initializer_list) is constexpr in C++17 onwards (tests/Dev11_1074023_constexpr). Mark libc++ std/language.support/support.initlist/support.initlist.range/begin_end.pass.cpp as known FAIL.
| # Copyright (c) Microsoft Corporation. | ||
| # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| RUNALL_INCLUDE ..\usual_latest_matrix.lst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes should be tested in C++14 mode.
| RUNALL_INCLUDE ..\usual_latest_matrix.lst | |
| RUNALL_INCLUDE ..\usual_matrix.lst |
Also, static_assert(e); is only standardized since C++17, so we need to write either static_assert(e, ""); or #define STATIC_ASSERT(__VA_ARGS__) static_assert(__VA_ARGS__, #__VA_ARGS__). The latter is widely used in MSVC STL's test suite.
stl/inc/yvals_core.h
Outdated
| // C++26 | ||
| #define __cpp_lib_initializer_list 202511L | ||
| #define __cpp_lib_valarray 202511L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be moved to the C++14 section. And we should test them in tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Individual parts of yvals_core.h are sorted alphabetically, right? So I should prefer alphabetical sorting over smaller diff?
Tests in tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp don't seem to be sorted, so I will just add new lines to the end.
| STATIC_ASSERT(il.begin() == il.end()); | ||
| #if _HAS_CXX17 | ||
| STATIC_ASSERT(begin(il) == end(il)); | ||
| #endif // _HAS_CXX17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For archaeology: constexpr for initializer_list was added to C++14 via WG21-N3471, while constexpr for free std::begin/std::end was added to C++17 via WG21-P0031R0.
I'm a bit hesitant now as the changes would disallow executing for (int n : {1, 2, 3}) { /* ... */ } in constant evaluation in C++14 mode, while WG21-N3471 intentionally allowed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No language lawyer here, so correct me if I am wrong, but the range-based for loop does not use the free function in this case, but rather the member begin/end. Those are marked as constexpr unconditionally (not only in C++14 onwards) in code.
Link to cppinsights C++14 mode (very powerful argument for normal developers, not sure whether it is appropriate for STL development 😅 ) https://cppinsights.io/s/faddcfbd
Nevertheless, I have added the for loop to the test suite and it compiles and executes in all modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No language lawyer here, so correct me if I am wrong, but the range-based for loop does not use the free function in this case, but rather the member begin/end. Those are marked as constexpr unconditionally (not only in C++14 onwards) in code.
Oh, you are right. Sorry. I must have been be confused with some constexpr reduction.
|
Status update (also for #5848): Your PRs are on my list of things to review (the STL Code Reviews project is my dashboard) but I will be delayed as I need to prioritize |
Remove free function overloads of
beginandendforinitializer_list.Replace free function overloads
emptyanddataforinitializer_listwith member functions.Replace free function overloads
beginandendforvalarraywith member functions.Provide new feature test macros
__cpp_lib_initializer_listand__cpp_lib_valarray.Resolves #5840