Implement constexpr algorithms. #425

BillyONeal · 2020-01-16T01:11:41Z

Resolves GH-6 ( P0202R3 ), GH-38 ( P0879R0 ), and drive-by fixes GH-414.

Constexprizes the following algorithms by adding constexpr, _CONSTEXPR20, and _CONSTEXPR20_ICE to things:

adjacent_find
all_of
any_of
binary_search
copy
copy_backward
copy_if
copy_n
count
count_if
equal
equal_range
exchange
fill
fill_n
find
find_end
find_first_of
find_if
find_if_not
for_each
for_each_n
generate
generate_n
includes
is_heap
is_heap
is_heap_until
is_partitioned
is_permutation
is_sorted
is_sorted_until
iter_swap
lexicographical_compare
lower_bound
make_heap
merge
mismatch
move
move_backward
next_permutation
none_of
nth_element
partial_sort
partial_sort_copy
partition
partition_copy
partition_point
pop_heap
prev_permutation
push_heap
remove
remove_copy
remove_copy_if
remove_if
replace
replace_copy
replace_copy_if
replace_if
reverse_copy
revese
rotate
rotate_copy
search
search_n
set_difference
set_intersection
set_symmetric_difference
set_union
sort
sort_heap
swap
swap_ranges
transform
unique
unique_copy
upper_bound

Specific notes:

skipped_tests.txt: Turn on all tests previously blocked by missing constexpr algorithms (and exchange and swap). Mark those algorithms that cannot be turned on that we have outstanding PRs for with their associated PRs.
yvals_core.h: Turn on feature test macros.
xutility: Move the _Ptr_cat family down to copy, and fix associated SHOUTY comments to indicate that this is really an implementation detail of copy, not something the rest of the standard library intends to use directly. Removed and clarified some of the comments as requested by Casey Carter.

Updates the llvm submodule to get llvm/llvm-project@6d8abe4

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

BillyONeal · 2020-01-16T01:13:47Z

FYI I haven't submitted an internal PR yet because I'm still working on adding tests, but wanted GH contributors to be able to see this when it was done.

CaseyCarter · 2020-01-16T01:31:45Z

stl/inc/xutility


-// ALGORITHM DISPATCH TRAITS
 template <class>
 class move_iterator;


Can this declaration move (pun intended) as well?

CaseyCarter · 2020-01-16T01:35:39Z

stl/inc/xutility


 // FUNCTION TEMPLATE copy
+template <class _Source, class _Dest>
+struct _Ptr_cat_helper { // determines _Ptr_cat's result in the most general case


All of these comments on _Ptr_cat_helper and its specializations should feed the flames, IMO.

Hmm I think on the partial specializations they serve to motivate why the partial specialization is OK. I removed some of the obvious ones.

To me, "determines _Ptr_cat's result" says that _Ptr_cat_helper implements _Ptr_cat - which is ~obvious - and each of the "when [condition]" bits simply repeat the pattern that the partial specialization matches:

_Ptr_cat_helper<_Elem, _Elem>..."when the types are the same"

_Ptr_cat_helper<_Anything*, const _Anything*>..."when all we do is add const to a pointer"

I don't think the comments provide any new information or justification.

EDIT: Not that it's worth wasting our time arguing about, either. Feel free to apply or ignore this comment and mark "Resolved"; I've said my piece ;)

stl/inc/xutility

CaseyCarter · 2020-01-16T01:47:46Z

stl/inc/xutility

+struct _Ptr_copy_cat<_Source*, _Dest*>
+    : conditional_t<is_trivially_assignable_v<_Dest&, _Source&>,
+          _Ptr_cat_helper<remove_cv_t<_Source>, remove_cv_t<_Dest>>, _False_copy_cat> {
+}; // return pointer copy optimization category for pointers


I thought we put the comment inside the {} when {}; // comment won't all fit on one line?

The red text contradicts my recollection. 🤷‍♂ Feel free to humor me or ignore this comment; I'm not sure I care either way.

If they go away completely it doesn't matter where they are :)

CaseyCarter · 2020-01-16T01:51:39Z

stl/inc/xutility

-_OutIt _Copy_unchecked(_InIt _First, _InIt _Last, _OutIt _Dest) {
+_CONSTEXPR20_ICE _OutIt _Copy_unchecked(_InIt _First, _InIt _Last, _OutIt _Dest) {
    // copy [_First, _Last) to [_Dest, ...)
    // note: _Copy_unchecked is called directly from elsewhere in the STL


Suggested change

// note: _Copy_unchecked is called directly from elsewhere in the STL

// note: _Copy_unchecked has callers other than copy

Ditto 3915 and 4006.

CaseyCarter · 2020-01-16T02:32:32Z

stl/inc/algorithm

-_NODISCARD pair<_InIt1, _RightTy*> mismatch(const _InIt1 _First1, const _InIt1 _Last1, _RightTy (&_First2)[_RightSize],
+_NODISCARD _CONSTEXPR20 pair<_InIt1, _RightTy*> mismatch(const _InIt1 _First1, const _InIt1 _Last1,
+    _RightTy (&_First2)[_RightSize],
    _Pr _Pred) { // return [_First1, _Last1)/[_First2, ...) mismatch using _Pred


Wrap comment?

stl/inc/algorithm

CaseyCarter · 2020-01-16T02:43:36Z

stl/inc/algorithm

-_OutIt transform(const _InIt1 _First1, const _InIt1 _Last1, _RightTy (&_First2)[_RightSize], const _OutIt _Dest,
+_CONSTEXPR20 _OutIt transform(const _InIt1 _First1, const _InIt1 _Last1, _RightTy (&_First2)[_RightSize],
+    const _OutIt _Dest,
    _Fn _Func) { // transform [_First1, _Last1) and [_First2, ...), array source


Wrap comment.

CaseyCarter · 2020-01-16T02:44:57Z

stl/inc/algorithm

 template <class _OutIt, class _Diff, class _Fn>
-_OutIt generate_n(_OutIt _Dest, const _Diff _Count_raw, _Fn _Func) { // replace [_Dest, _Dest + _Count) with _Func()
+_CONSTEXPR20 _OutIt generate_n(
+    _OutIt _Dest, const _Diff _Count_raw, _Fn _Func) { // replace [_Dest, _Dest + _Count) with _Func()


Wrap comment.

CaseyCarter · 2020-01-16T02:45:11Z

stl/inc/algorithm

 template <class _InIt, class _OutIt, class _Ty>
-_OutIt remove_copy(_InIt _First, _InIt _Last, _OutIt _Dest, const _Ty& _Val) { // copy omitting each matching _Val
+_CONSTEXPR20 _OutIt remove_copy(
+    _InIt _First, _InIt _Last, _OutIt _Dest, const _Ty& _Val) { // copy omitting each matching _Val


stl/inc/xutility

BillyONeal · 2020-01-16T23:49:59Z

FYI to reviewers, I'm going to wait till #423 merges to resolve comments here.

Resolves microsoftGH-6 ( P0202R3 ), microsoftGH-38 ( P0879R0 ), and drive-by fixes microsoftGH-414. Everywhere: Add constexpr, _CONSTEXPR20, and _CONSTEXPR20_ICE to things. skipped_tests.txt: Turn on all tests previously blocked by missing constexpr algorithms (and exchange and swap). Mark those algorithms that cannot be turned on that we have outstanding PRs for with their associated PRs. yvals_core.h: Turn on feature test macros. xutility: * Move the _Ptr_cat family down to copy, and fix associated SHOUTY comments to indicate that this is really an implementation detail of copy, not something the rest of the standard library intends to use directly. Removed and clarified some of the comments as requested by Casey Carter. * Extract _Copy_n_core which implements copy_n using only the core language (rather than memcpy-as-an-intrinsic). Note that we cannot use __builtin_memcpy or similar to avoid the is_constant_evaluated check here; builtin_memcpy only works in constexpr contexts when the inputs are of type char. numeric: Refactor as suggested by microsoftGH-414.

StephanTLavavej · 2020-01-21T23:10:22Z

stl/inc/yvals_core.h

 #define __cpp_lib_to_array                201907L
 #define __cpp_lib_type_identity           201806L
 #define __cpp_lib_unwrap_ref              201811L



The newly-implemented features need to be listed in the comment at the top.

StephanTLavavej · 2020-01-21T23:15:22Z

stl/inc/numeric

-        } else {
+        if (_STD is_constant_evaluated()) {
            for (; _UFirst != _ULast; ++_UFirst) {
                _Val = _Reduce_op(_STD move(_Val), *_UFirst); // Requirement missing from N4713


This repeats the "classic implementation"; for #414 I hoped that we would be able to avoid that. Did you intend to choose another approach?

Nothing suggested in #414 avoids repeating the classic implementation, only moving it such only one copy of it is considered for a given setting of the if constexpr branch.

I suggested avoiding the repetition. See:

So perhaps the best pattern is:

if constexpr (_Plus_on_arithmetic_ranges_reduction_v) { if (!_STD is_constant_evaluated()) { return _Reduce_plus_arithmetic_ranges(ARGS); } } return CLASSIC_IMPLEMENTATION;

I guess when you filed that you made it unrelated to the original submission.

I believe that pattern will trip DevCom-889321.

The proposed pattern is is_constant_evaluated within if constexpr; your filed bug is about if constexpr within is_constant_evaluated.

OTOH DevCom-889321 was the opposite (if constexpr inside is_constant_evaluated). Let me try this and see if it works out...

Hmmm well it seems to work so I'm not going to look a gift horse in the mouth ¯\_(ツ)_/¯

StephanTLavavej · 2020-01-21T23:22:49Z

stl/inc/algorithm

 // FUNCTION TEMPLATE swap_ranges
 template <class _FwdIt1, class _FwdIt2>
-_FwdIt2 _Swap_ranges_unchecked(_FwdIt1 _First1, const _FwdIt1 _Last1, _FwdIt2 _First2) {
+constexpr _FwdIt2 _Swap_ranges_unchecked(_FwdIt1 _First1, const _FwdIt1 _Last1, _FwdIt2 _First2) {


Why is this plain constexpr? This calls iter_swap which is _CONSTEXPR20.

I guess nobody cares about that IFNDR

StephanTLavavej · 2020-01-21T23:28:46Z

stl/inc/algorithm


 // FUNCTION TEMPLATE merge
-inline _Distance_unknown _Idl_dist_add(_Distance_unknown, _Distance_unknown) {
+constexpr _Distance_unknown _Idl_dist_add(_Distance_unknown, _Distance_unknown) {


Pre-existing: The _Idl_dist_add overloads should be _NODISCARD.

stl/inc/xutility

StephanTLavavej · 2020-01-21T23:41:53Z

stl/inc/xutility


 template <class _FwdIt1, class _FwdIt2>
-bool is_permutation(_FwdIt1 _First1, _FwdIt1 _Last1, _FwdIt2 _First2) {
+_CONSTEXPR20 bool is_permutation(_FwdIt1 _First1, _FwdIt1 _Last1, _FwdIt2 _First2) {


Pre-existing: This Standard overload is egregiously and mysteriously missing _NODISCARD.

* _Swap_ranges_unchecked => _CONSTEXPR20 * _Idl_dist_add => _NODISCARD (and remove DinkumComments) * is_permutation => _NODISCARD * Add yvals_core.h comments.

StephanTLavavej · 2020-01-22T03:53:12Z

stl/inc/numeric

-        }
-        return _Val;
-    } else
+        // TRANSITION, DevCom-878972


Is this still TRANSITION now that you've eliminated the code duplication?

StephanTLavavej · 2020-01-22T03:54:06Z

stl/inc/xutility


 // FUNCTION TEMPLATE copy_n
+template <class _InIt, class _Diff, class _OutIt>
+constexpr _OutIt _Copy_n_core(_InIt _First, _Diff _Count, _OutIt _Dest) {}


_Copy_n_core appears to be unused, and it has an empty function body.

stl/inc/yvals_core.h

stl/inc/xutility

StephanTLavavej

Looks good, thanks for working on this!

StephanTLavavej · 2020-01-23T01:58:06Z

Microsoft-internal MSVC-PR-224184 completed, so I went ahead and merged this.

In GH-425 I was forced to add a dead initialization for the _Count variable in _Sort_unchecked in order to comply with constexpr rules. Also, _Sort_unchecked had somewhat complex assignment-in-conditional-expressions going on. This change moves the code around such that the _Count variable is assigned once. Also: * Consistently test _ISORT_MAX with <= where possible, and make that an _INLINE_VAR constexpr variable. * Remove _Count guards of 1 in front of _Insertion_sort_unchecked for consistency. I did performance testing and there was no measurable difference in keeping this check, and it's more code to reason about. * Avoid needless casts of _ISORT_MAX given that it is now a constexpr constant.

BillyONeal requested a review from a team as a code owner January 16, 2020 01:11

BillyONeal self-assigned this Jan 16, 2020

CaseyCarter suggested changes Jan 16, 2020

View reviewed changes

miscco reviewed Jan 16, 2020

View reviewed changes

stl/inc/xutility Show resolved Hide resolved

miscco reviewed Jan 16, 2020

View reviewed changes

stl/inc/xutility Outdated Show resolved Hide resolved

kangzixiang approved these changes Jan 16, 2020

View reviewed changes

This was referenced Jan 17, 2020

Optimize the is_permutation family and _Hash::operator== for multicontainers #423

Merged

<xutility>: copy and fill family could engage memcpy / memset in more cases #431

Closed

BillyONeal force-pushed the constexpr_algorithm branch from c09c7d3 to ad2c84f Compare January 21, 2020 23:44

StephanTLavavej requested changes Jan 21, 2020

View reviewed changes

BillyONeal added 2 commits January 21, 2020 19:42

Attempt alternate fix of microsoftGH-414 suggested by Stephan.

88ec800

Stephan product code PR comments:

b10ada4

* _Swap_ranges_unchecked => _CONSTEXPR20 * _Idl_dist_add => _NODISCARD (and remove DinkumComments) * is_permutation => _NODISCARD * Add yvals_core.h comments.

StephanTLavavej requested changes Jan 22, 2020

View reviewed changes

Delete unused _Copy_n_core and TRANSITION, DevCom-889321 comment.

e307d2f

BillyONeal requested review from CaseyCarter and StephanTLavavej January 22, 2020 04:23

StephanTLavavej requested changes Jan 22, 2020

View reviewed changes

stl/inc/yvals_core.h Outdated Show resolved Hide resolved

stl/inc/xutility Outdated Show resolved Hide resolved

stl/inc/xutility Outdated Show resolved Hide resolved

Put the comments in the right place and remove phantom braces.

be0738f

BillyONeal requested a review from StephanTLavavej January 22, 2020 20:06

StephanTLavavej approved these changes Jan 22, 2020

View reviewed changes

CaseyCarter approved these changes Jan 22, 2020

View reviewed changes

StephanTLavavej merged commit 3447e56 into microsoft:master Jan 23, 2020

BillyONeal deleted the constexpr_algorithm branch January 23, 2020 04:29

This was referenced Jan 23, 2020

Remove dead store to _Count in _Sort_unchecked #445

Closed

Remove dead store to _Count in _Sort_unchecked and other cleanups #449

Merged

	// note: _Copy_unchecked is called directly from elsewhere in the STL
	// note: _Copy_unchecked has callers other than copy

Implement constexpr algorithms. #425

Implement constexpr algorithms. #425

Uh oh!

Conversation

BillyONeal commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

BillyONeal commented Jan 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaseyCarter Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BillyONeal commented Jan 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillyONeal commented Jan 16, 2020 •

edited

Loading

CaseyCarter Jan 21, 2020 •

edited

Loading

BillyONeal Jan 22, 2020 •

edited

Loading

BillyONeal Jan 22, 2020 •

edited

Loading