Skip to content

Commit 8b394f7

Browse files
ldionneldionne
committed
[libc++][pstl] Improve exception handling
There were various places where we incorrectly handled exceptions in the PSTL. Typical issues were missing `noexcept` and taking iterators by value instead of by reference. This patch fixes those inconsistent and incorrect instances, and adds proper tests for all of those. Note that the previous tests were often incorrectly turned into no-ops by the compiler due to copy ellision, which doesn't happen with these new tests.
1 parent d57907d commit 8b394f7

29 files changed

+404
-915
lines changed

‎libcxx/include/__algorithm/pstl_copy.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,12 @@ template <class _ExecutionPolicy,
9595
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_copy_n, _RawPolicy),
9696
[&__policy](
9797
_ForwardIterator __g_first, _Size __g_n, _ForwardOutIterator __g_result) -> optional<_ForwardIterator> {
98-
if constexpr (__has_random_access_iterator_category_or_concept<_ForwardIterator>::value)
98+
if constexpr (__has_random_access_iterator_category_or_concept<_ForwardIterator>::value) {
9999
return std::__copy(__policy, std::move(__g_first), std::move(__g_first + __g_n), std::move(__g_result));
100-
else
100+
} else {
101+
(void)__policy;
101102
return std::copy_n(__g_first, __g_n, __g_result);
103+
}
102104
},
103105
std::move(__first),
104106
std::move(__n),

‎libcxx/include/__algorithm/pstl_count.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,8 @@ template <class _ExecutionPolicy,
8787
class _Tp,
8888
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
8989
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
90-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__iter_diff_t<_ForwardIterator>>
91-
__count(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, const _Tp& __value) {
90+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__iter_diff_t<_ForwardIterator>> __count(
91+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, const _Tp& __value) noexcept {
9292
return std::__pstl_frontend_dispatch(
9393
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_count, _RawPolicy),
9494
[&](_ForwardIterator __g_first, _ForwardIterator __g_last, const _Tp& __g_value)
@@ -97,8 +97,8 @@ __count(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator
9797
return __v == __g_value;
9898
});
9999
},
100-
std::move(__first),
101-
std::move(__last),
100+
std::forward<_ForwardIterator>(__first),
101+
std::forward<_ForwardIterator>(__last),
102102
__value);
103103
}
104104

‎libcxx/include/__algorithm/pstl_equal.h

+8-2
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,10 @@ _LIBCPP_HIDE_FROM_ABI bool
9191
equal(_ExecutionPolicy&& __policy, _ForwardIterator1 __first1, _ForwardIterator1 __last1, _ForwardIterator2 __first2) {
9292
_LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator1, "equal requires ForwardIterators");
9393
_LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator2, "equal requires ForwardIterators");
94-
return std::equal(__policy, std::move(__first1), std::move(__last1), std::move(__first2), std::equal_to{});
94+
auto __res = std::__equal(__policy, std::move(__first1), std::move(__last1), std::move(__first2), std::equal_to{});
95+
if (!__res)
96+
std::__throw_bad_alloc();
97+
return *__res;
9598
}
9699

97100
template <class _ExecutionPolicy,
@@ -171,8 +174,11 @@ equal(_ExecutionPolicy&& __policy,
171174
_ForwardIterator2 __last2) {
172175
_LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator1, "equal requires ForwardIterators");
173176
_LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator2, "equal requires ForwardIterators");
174-
return std::equal(
177+
auto __res = std::__equal(
175178
__policy, std::move(__first1), std::move(__last1), std::move(__first2), std::move(__last2), std::equal_to{});
179+
if (!__res)
180+
std::__throw_bad_alloc();
181+
return *__res;
176182
}
177183

178184
_LIBCPP_END_NAMESPACE_STD

‎libcxx/include/__algorithm/pstl_fill.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,17 @@ template <class _ExecutionPolicy,
4141
class _Tp,
4242
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
4343
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
44-
_LIBCPP_HIDE_FROM_ABI optional<__empty>
45-
__fill(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, const _Tp& __value) noexcept {
44+
_LIBCPP_HIDE_FROM_ABI optional<__empty> __fill(
45+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, const _Tp& __value) noexcept {
4646
return std::__pstl_frontend_dispatch(
4747
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_fill, _RawPolicy),
4848
[&](_ForwardIterator __g_first, _ForwardIterator __g_last, const _Tp& __g_value) {
4949
return std::__for_each(__policy, __g_first, __g_last, [&](__iter_reference<_ForwardIterator> __element) {
5050
__element = __g_value;
5151
});
5252
},
53-
std::move(__first),
54-
std::move(__last),
53+
std::forward<_ForwardIterator>(__first),
54+
std::forward<_ForwardIterator>(__last),
5555
__value);
5656
}
5757

‎libcxx/include/__algorithm/pstl_find.h

+9-9
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,8 @@ template <class _ExecutionPolicy,
6565
class _Predicate,
6666
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
6767
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
68-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__remove_cvref_t<_ForwardIterator>>
69-
__find_if_not(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Predicate&& __pred) {
68+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__remove_cvref_t<_ForwardIterator>> __find_if_not(
69+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Predicate&& __pred) noexcept {
7070
return std::__pstl_frontend_dispatch(
7171
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_find_if_not, _RawPolicy),
7272
[&](_ForwardIterator&& __g_first, _ForwardIterator&& __g_last, _Predicate&& __g_pred)
@@ -76,9 +76,9 @@ __find_if_not(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardI
7676
return !__g_pred(__value);
7777
});
7878
},
79-
std::move(__first),
80-
std::move(__last),
81-
std::move(__pred));
79+
std::forward<_ForwardIterator>(__first),
80+
std::forward<_ForwardIterator>(__last),
81+
std::forward<_Predicate>(__pred));
8282
}
8383

8484
template <class _ExecutionPolicy,
@@ -103,8 +103,8 @@ template <class _ExecutionPolicy,
103103
class _Tp,
104104
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
105105
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
106-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__remove_cvref_t<_ForwardIterator>>
107-
__find(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, const _Tp& __value) noexcept {
106+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__remove_cvref_t<_ForwardIterator>> __find(
107+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, const _Tp& __value) noexcept {
108108
return std::__pstl_frontend_dispatch(
109109
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_find, _RawPolicy),
110110
[&](_ForwardIterator __g_first, _ForwardIterator __g_last, const _Tp& __g_value) -> optional<_ForwardIterator> {
@@ -113,8 +113,8 @@ __find(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator _
113113
return __element == __g_value;
114114
});
115115
},
116-
std::move(__first),
117-
std::move(__last),
116+
std::forward<_ForwardIterator>(__first),
117+
std::forward<_ForwardIterator>(__last),
118118
__value);
119119
}
120120

‎libcxx/include/__algorithm/pstl_generate.h

+3-3
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ template <class _ExecutionPolicy,
4040
class _Generator,
4141
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
4242
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
43-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty>
44-
__generate(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Generator&& __gen) {
43+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty> __generate(
44+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Generator&& __gen) noexcept {
4545
return std::__pstl_frontend_dispatch(
4646
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_generate, _RawPolicy),
4747
[&__policy](_ForwardIterator __g_first, _ForwardIterator __g_last, _Generator __g_gen) {
@@ -77,7 +77,7 @@ template <class _ExecutionPolicy,
7777
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
7878
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
7979
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty>
80-
__generate_n(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _Size&& __n, _Generator&& __gen) {
80+
__generate_n(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _Size&& __n, _Generator&& __gen) noexcept {
8181
return std::__pstl_frontend_dispatch(
8282
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_generate_n, _RawPolicy),
8383
[&__policy](_ForwardIterator __g_first, _Size __g_n, _Generator __g_gen) {

‎libcxx/include/__algorithm/pstl_is_partitioned.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ template <class _ExecutionPolicy,
4141
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
4242
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
4343
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<bool> __is_partitioned(
44-
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Predicate&& __pred) {
44+
_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Predicate&& __pred) noexcept {
4545
return std::__pstl_frontend_dispatch(
4646
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_is_partitioned, _RawPolicy),
4747
[&__policy](_ForwardIterator __g_first, _ForwardIterator __g_last, _Predicate __g_pred) {

‎libcxx/include/__algorithm/pstl_merge.h

+14-13
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include <__type_traits/enable_if.h>
1717
#include <__type_traits/is_execution_policy.h>
1818
#include <__type_traits/remove_cvref.h>
19+
#include <__utility/forward.h>
1920
#include <__utility/move.h>
2021
#include <optional>
2122

@@ -34,26 +35,26 @@ template <class _ExecutionPolicy,
3435
class _ForwardIterator1,
3536
class _ForwardIterator2,
3637
class _ForwardOutIterator,
37-
class _Comp = std::less<>,
38+
class _Comp,
3839
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
3940
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
4041
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<_ForwardOutIterator>
4142
__merge(_ExecutionPolicy&&,
42-
_ForwardIterator1 __first1,
43-
_ForwardIterator1 __last1,
44-
_ForwardIterator2 __first2,
45-
_ForwardIterator2 __last2,
46-
_ForwardOutIterator __result,
47-
_Comp __comp = {}) noexcept {
43+
_ForwardIterator1&& __first1,
44+
_ForwardIterator1&& __last1,
45+
_ForwardIterator2&& __first2,
46+
_ForwardIterator2&& __last2,
47+
_ForwardOutIterator&& __result,
48+
_Comp&& __comp) noexcept {
4849
using _Backend = typename __select_backend<_RawPolicy>::type;
4950
return std::__pstl_merge<_RawPolicy>(
5051
_Backend{},
51-
std::move(__first1),
52-
std::move(__last1),
53-
std::move(__first2),
54-
std::move(__last2),
55-
std::move(__result),
56-
std::move(__comp));
52+
std::forward<_ForwardIterator1>(__first1),
53+
std::forward<_ForwardIterator1>(__last1),
54+
std::forward<_ForwardIterator2>(__first2),
55+
std::forward<_ForwardIterator2>(__last2),
56+
std::forward<_ForwardOutIterator>(__result),
57+
std::forward<_Comp>(__comp));
5758
}
5859

5960
template <class _ExecutionPolicy,

‎libcxx/include/__algorithm/pstl_replace.h

+5-5
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ template <class _ExecutionPolicy,
9191
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
9292
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty>
9393
__replace(_ExecutionPolicy&& __policy,
94-
_ForwardIterator __first,
95-
_ForwardIterator __last,
94+
_ForwardIterator&& __first,
95+
_ForwardIterator&& __last,
9696
const _Tp& __old_value,
9797
const _Tp& __new_value) noexcept {
9898
return std::__pstl_frontend_dispatch(
@@ -106,8 +106,8 @@ __replace(_ExecutionPolicy&& __policy,
106106
[&](__iter_reference<_ForwardIterator> __element) { return __element == __g_old_value; },
107107
__g_new_value);
108108
},
109-
std::move(__first),
110-
std::move(__last),
109+
std::forward<_ForwardIterator>(__first),
110+
std::forward<_ForwardIterator>(__last),
111111
__old_value,
112112
__new_value);
113113
}
@@ -144,7 +144,7 @@ template <class _ExecutionPolicy,
144144
_ForwardIterator&& __last,
145145
_ForwardOutIterator&& __result,
146146
_Pred&& __pred,
147-
const _Tp& __new_value) {
147+
const _Tp& __new_value) noexcept {
148148
return std::__pstl_frontend_dispatch(
149149
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_replace_copy_if, _RawPolicy),
150150
[&__policy](_ForwardIterator __g_first,

‎libcxx/include/__algorithm/pstl_sort.h

+10-6
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,20 @@ template <class _ExecutionPolicy,
4141
class _Comp,
4242
class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>,
4343
enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
44-
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty> __sort(
45-
_ExecutionPolicy&& __policy, _RandomAccessIterator __first, _RandomAccessIterator __last, _Comp __comp) noexcept {
44+
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<__empty>
45+
__sort(_ExecutionPolicy&& __policy,
46+
_RandomAccessIterator&& __first,
47+
_RandomAccessIterator&& __last,
48+
_Comp&& __comp) noexcept {
4649
return std::__pstl_frontend_dispatch(
4750
_LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_sort, _RawPolicy),
4851
[&__policy](_RandomAccessIterator __g_first, _RandomAccessIterator __g_last, _Comp __g_comp) {
4952
std::stable_sort(__policy, std::move(__g_first), std::move(__g_last), std::move(__g_comp));
5053
return optional<__empty>{__empty{}};
5154
},
52-
std::move(__first),
53-
std::move(__last),
54-
std::move(__comp));
55+
std::forward<_RandomAccessIterator>(__first),
56+
std::forward<_RandomAccessIterator>(__last),
57+
std::forward<_Comp>(__comp));
5558
}
5659

5760
template <class _ExecutionPolicy,
@@ -73,7 +76,8 @@ template <class _ExecutionPolicy,
7376
_LIBCPP_HIDE_FROM_ABI void
7477
sort(_ExecutionPolicy&& __policy, _RandomAccessIterator __first, _RandomAccessIterator __last) {
7578
_LIBCPP_REQUIRE_CPP17_RANDOM_ACCESS_ITERATOR(_RandomAccessIterator, "sort requires RandomAccessIterators");
76-
std::sort(std::forward<_ExecutionPolicy>(__policy), std::move(__first), std::move(__last), less{});
79+
if (!std::__sort(__policy, std::move(__first), std::move(__last), less{}))
80+
std::__throw_bad_alloc();
7781
}
7882

7983
_LIBCPP_END_NAMESPACE_STD

‎libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.exception_handling.pass.cpp

-58
This file was deleted.

‎libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp

-40
This file was deleted.

0 commit comments

Comments
 (0)