Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Oct 20, 2020

This implements remaining three stable algorithms. (Works towards #39.)

I am slightly worried about inplace_merge as I did some miniscule simplifications here and there and there are soo many .

Also I really do not like all those free functions flying around. Would it make sense to keep them as static member functions of the respective CPO and only declare those that are used outside as public?

@miscco miscco requested a review from a team as a code owner October 20, 2020 08:27
Comment on lines +6089 to +6090
_Uninitialized_backout<iter_value_t<_It>*> _Backout{
_Temp_ptr, _RANGES _Uninitialized_move_unchecked(_First, _Mid, _Temp_ptr, _Temp_ptr + _Count1).out};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not like this function call as function argument.

I will most likely go over this soonish and give the result proper names

@miscco
Copy link
Contributor Author

miscco commented Oct 20, 2020

We are not using some of the return values from helper algorithms, such as _Move_backward_common

I have for now removed the nodiscard here as it seems that this algorithm might also be a good candidate for not using nodiscard

@miscco miscco force-pushed the ranges_stable_partition branch from 7ec7acd to d343edd Compare October 20, 2020 10:17
@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Oct 20, 2020
@CaseyCarter CaseyCarter mentioned this pull request Oct 20, 2020
@CaseyCarter CaseyCarter self-assigned this Oct 21, 2020
@miscco miscco changed the title Implement ranges::stable partition and ranges::inplace_merge Implement allocating algorithms for ranges Oct 23, 2020
@miscco
Copy link
Contributor Author

miscco commented Oct 23, 2020

Added stable_sort

@miscco
Copy link
Contributor Author

miscco commented Oct 25, 2020

FYI I tried to unify this using if constexpr but it did not really improve readability.

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

Co-authored-by: Casey Carter <cartec69@gmail.com>
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Through inplace_merge.

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, still need to look at tests.

@miscco miscco force-pushed the ranges_stable_partition branch from 17057b8 to 68b6544 Compare November 7, 2020 20:51
miscco and others added 6 commits November 19, 2020 22:23
* `_Merge_move_to_buffer` shouldn't use `_Uninitialized_backout`; doing so could result in this function destroying a range of objects in the middle of the temporary buffer on exception from `iter_swap`.
* `_Merge_move_to_buffer` shouldn't be using `iter_swap` anyway since we don't care about the pre-existing values in the destination range; it should indirectly_moving a la `*out = ranges::iter_move(in)`.
* With the above changes, `_Merge_move_to_buffer` and `_Merge_move_from_buffer` can be refactored into a single function `_Merge_move_common`.
* .`_Chunked_merge_to_buffer` and `_Chunked_merge_from_buffer` are then identical, and can be refactored into a single function `_Chunked_merge_common`.
@CaseyCarter CaseyCarter self-requested a review January 22, 2021 21:04
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of copypasta I'll apply fixes for, and one comment question.

@CaseyCarter CaseyCarter removed their assignment Jan 25, 2021
Cleanup comment copypasta
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just found various nitpicks and a question about comment accuracy.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with 3 unresolved questions about comment accuracy.

CaseyCarter and others added 2 commits January 26, 2021 15:09
* Promote `_Isort_max` to namespace `std`; use it where appropriate in `<algorithm>` and `<execution>`.

* Don't say "not less than" when we mean "greater than".

* Refactor duplicate code into a `do`-`while` loop in `ranges::_Buffered_inplace_merge_common`.

* Constant-propagate the value of `_Chunk` (`_Isort_max<_BidIt>`) through the body of `_Uninitialized_chunked_merge_unchecked`, remove the parameter, and bump the name to `_Uninitialized_chunked_merge_unchecked2` for ABI.
Say "Validate empty range" consistently instead of sometimes "ranges".
@StephanTLavavej StephanTLavavej self-assigned this Jan 27, 2021
@StephanTLavavej StephanTLavavej merged commit a5b13ff into microsoft:master Jan 27, 2021
@StephanTLavavej
Copy link
Member

For implementing these algorithms, I thank you. Most complicated in the classic STL, they were. 👽 ⚙️

@miscco miscco deleted the ranges_stable_partition branch January 27, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants