Skip to content
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

parallel_sort requires lvalue range, thus cannot be used rlavue ranges (which is a common use case in our codebase) #500

Closed
sf-mc opened this issue Jul 20, 2021 · 3 comments · Fixed by #605
Assignees

Comments

@sf-mc
Copy link

sf-mc commented Jul 20, 2021

This bug was introduced in commit 4cebdd9.

There was removed
void parallel_sort(const Range& rng, const Compare& comp) {
and
void parallel_sort(const Range& rng) {

Probably it was done by mistake, with intention to instead remove the redundant
void parallel_sort(Range& rng, const Compare& comp) {
and
void parallel_sort(Range& rng) {

The bug is still present in the latest version, see
https://github.com/oneapi-src/oneTBB/blob/master/include/oneapi/tbb/parallel_sort.h#L253

@sf-mc sf-mc changed the title parallel_sort requires lvalue range, thus cannot be used rlavue ranges (which is a common use in our codebase) parallel_sort requires lvalue range, thus cannot be used rlavue ranges (which is a common use case in our codebase) Jul 20, 2021
@sf-mc sf-mc closed this as completed Jul 28, 2021
@sf-mc sf-mc reopened this Jul 28, 2021
@sf-mc
Copy link
Author

sf-mc commented Jul 28, 2021

Some clarification.
void parallel_sort(Range& rng) {
is required for ranges like std::vector, because their begin() const returns const iterator.
And
void parallel_sort(const Range& rng) {
is needed for rvalue ranges like std::span, with usage like

std::span<int> get_span();
tbb::parallel_sort(get_span());

@ivankochin
Copy link
Contributor

Hello @sf-mc,
We may add the overload for the rvalue reference. Would this help in your case?

@sf-mc
Copy link
Author

sf-mc commented Oct 1, 2021

Hello @ivankochin,
Yes, it will help.
It is also worth to add such use case (rvalue ranges like std::span) to your unit test, so as to prevent people from deleting such overload by mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants