Check the swappable requirement for the parallel_sort algorithm #656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
November 15, 2021 15:08
… test with custom swap Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
…ng tests Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
added 3 commits
November 16, 2021 09:48
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
ivankochin
force-pushed
the
dev/ivankochin/parallel-sort-check-swappable-req
branch
from
50c4fdd to
594a6ad
Compare
kboyarinov
reviewed
Nov 17, 2021
include/oneapi/tbb/parallel_sort.h
Outdated
| void parallel_sort( RandomAccessIterator begin, RandomAccessIterator end ) { | ||
| parallel_sort(begin, end, std::less<typename std::iterator_traits<RandomAccessIterator>::value_type>()); | ||
| } | ||
|
|
||
| template<typename Range> | ||
| using range_value_type = typename std::iterator_traits<range_iterator_type<Range>>::value_type; |
There was a problem hiding this comment.
May be we can move this helper to _range_common.h and simplify tbb::parallel_for_each constraints? The same for iter_value_type
There was a problem hiding this comment.
I don't think so, As I know the parallel_for_each algorithm checks the requirements on reference types instead of value types.
test/common/concepts_common.h
Outdated
|
|
||
| template <bool EnableBegin, bool EnableEnd> | ||
| using NonSwappableValue = ParallelSortValue</*MovableV = */true, /*MoveAssignableV = */true, /*ComparableV = */true>; |
There was a problem hiding this comment.
It seems like ParallelSortValue<true, true, true> is swappable...
There was a problem hiding this comment.
Yes, I understand it and don't use this value in testing. So I just forgot to remove it from here. Thanks!
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
kboyarinov
reviewed
Dec 6, 2021
include/oneapi/tbb/parallel_sort.h
Outdated
| @@ -240,7 +250,9 @@ void parallel_sort( RandomAccessIterator begin, RandomAccessIterator end, const | |||
| /** @ingroup algorithms **/ | |||
| template<typename RandomAccessIterator> | |||
| __TBB_requires(std::random_access_iterator<RandomAccessIterator> && | |||
| less_than_comparable<typename std::iterator_traits<RandomAccessIterator>::value_type>) | |||
| less_than_comparable<iter_value_type<RandomAccessIterator>> && | |||
| std::swappable<iter_value_type<RandomAccessIterator>> && | |||
There was a problem hiding this comment.
Seems unnecessary, same as above
include/oneapi/tbb/parallel_sort.h
Outdated
| @@ -258,7 +272,9 @@ void parallel_sort( Range&& rng, const Compare& comp ) { | |||
| /** @ingroup algorithms **/ | |||
| template<typename Range> | |||
| __TBB_requires(container_based_sequence<Range, std::random_access_iterator_tag> && | |||
| less_than_comparable<typename std::iterator_traits<range_iterator_type<Range>>::value_type>) | |||
| less_than_comparable<range_value_type<Range>> && | |||
| std::swappable<range_value_type<Range>> && | |||
There was a problem hiding this comment.
Seems unnecessary, same as above
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
ivankochin
deleted the
dev/ivankochin/parallel-sort-check-swappable-req
branch
kboyarinov
pushed a commit
that referenced
this pull request
Dec 27, 2021
Signed-off-by: Kochin Ivan <kochin.ivan@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The oneTBB spec contains the swappable requirement for the iterators, but this requirement isn't handled by the C++ constraints in the implementation. This patch enables these constraints for all the overloads to improve the diagnostic and also extends the testing.
Type of change
Tests
Documentation
uxlfoundation/oneAPI-spec#391
Breaks backward compatibility
Notify the following users
@kboyarinov
Signed-off-by: Kochin Ivan kochin.ivan@intel.com