[libc++][ranges] use static operator() for C++23 ranges
#86052
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Xiaoyang Liu (xiaoyang-sde) ChangesAbstractThis pull request converts the ReferenceFull diff: https://github.com/llvm/llvm-project/pull/86052.diff 7 Files Affected:
diff --git a/libcxx/include/__algorithm/ranges_ends_with.h b/libcxx/include/__algorithm/ranges_ends_with.h
index c2a3cae9f3b16a..bb01918326b8bc 100644
--- a/libcxx/include/__algorithm/ranges_ends_with.h
+++ b/libcxx/include/__algorithm/ranges_ends_with.h
@@ -39,7 +39,7 @@ namespace ranges {
namespace __ends_with {
struct __fn {
template <class _Iter1, class _Sent1, class _Iter2, class _Sent2, class _Pred, class _Proj1, class _Proj2>
- static _LIBCPP_HIDE_FROM_ABI constexpr bool __ends_with_fn_impl_bidirectional(
+ _LIBCPP_HIDE_FROM_ABI static constexpr bool __ends_with_fn_impl_bidirectional(
_Iter1 __first1,
_Sent1 __last1,
_Iter2 __first2,
@@ -56,7 +56,7 @@ struct __fn {
}
template <class _Iter1, class _Sent1, class _Iter2, class _Sent2, class _Pred, class _Proj1, class _Proj2>
- static _LIBCPP_HIDE_FROM_ABI constexpr bool __ends_with_fn_impl(
+ _LIBCPP_HIDE_FROM_ABI static constexpr bool __ends_with_fn_impl(
_Iter1 __first1,
_Sent1 __last1,
_Iter2 __first2,
@@ -65,7 +65,7 @@ struct __fn {
_Proj1& __proj1,
_Proj2& __proj2) {
if constexpr (std::bidirectional_iterator<_Sent1> && std::bidirectional_iterator<_Sent2> &&
- (!std::random_access_iterator<_Sent1>)&&(!std::random_access_iterator<_Sent2>)) {
+ (!std::random_access_iterator<_Sent1>) && (!std::random_access_iterator<_Sent2>)) {
return __ends_with_fn_impl_bidirectional(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
} else {
diff --git a/libcxx/include/__algorithm/ranges_starts_with.h b/libcxx/include/__algorithm/ranges_starts_with.h
index 90e184aa9bccc2..7ba8af13a8d1c8 100644
--- a/libcxx/include/__algorithm/ranges_starts_with.h
+++ b/libcxx/include/__algorithm/ranges_starts_with.h
@@ -42,14 +42,14 @@ struct __fn {
class _Proj1 = identity,
class _Proj2 = identity>
requires indirectly_comparable<_Iter1, _Iter2, _Pred, _Proj1, _Proj2>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr bool operator()(
_Iter1 __first1,
_Sent1 __last1,
_Iter2 __first2,
_Sent2 __last2,
_Pred __pred = {},
_Proj1 __proj1 = {},
- _Proj2 __proj2 = {}) const {
+ _Proj2 __proj2 = {}) {
return __mismatch::__fn::__go(
std::move(__first1),
std::move(__last1),
@@ -67,8 +67,8 @@ struct __fn {
class _Proj1 = identity,
class _Proj2 = identity>
requires indirectly_comparable<iterator_t<_Range1>, iterator_t<_Range2>, _Pred, _Proj1, _Proj2>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(
- _Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr bool
+ operator()(_Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) {
return __mismatch::__fn::__go(
ranges::begin(__range1),
ranges::end(__range1),
diff --git a/libcxx/include/__ranges/as_rvalue_view.h b/libcxx/include/__ranges/as_rvalue_view.h
index 295aa94ed9fe40..b4a04e8fd81509 100644
--- a/libcxx/include/__ranges/as_rvalue_view.h
+++ b/libcxx/include/__ranges/as_rvalue_view.h
@@ -111,7 +111,7 @@ namespace views {
namespace __as_rvalue {
struct __fn : __range_adaptor_closure<__fn> {
template <class _Range>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Range&& __range)
noexcept(noexcept(/**/ as_rvalue_view(std::forward<_Range>(__range))))
-> decltype(/*--*/ as_rvalue_view(std::forward<_Range>(__range))) {
return /*-------------*/ as_rvalue_view(std::forward<_Range>(__range));
@@ -119,7 +119,7 @@ struct __fn : __range_adaptor_closure<__fn> {
template <class _Range>
requires same_as<range_rvalue_reference_t<_Range>, range_reference_t<_Range>>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Range&& __range)
noexcept(noexcept(/**/ views::all(std::forward<_Range>(__range))))
-> decltype(/*--*/ views::all(std::forward<_Range>(__range))) {
return /*-------------*/ views::all(std::forward<_Range>(__range));
diff --git a/libcxx/include/__ranges/chunk_by_view.h b/libcxx/include/__ranges/chunk_by_view.h
index b04a23de99fb2a..aaa855e6a276f7 100644
--- a/libcxx/include/__ranges/chunk_by_view.h
+++ b/libcxx/include/__ranges/chunk_by_view.h
@@ -205,7 +205,7 @@ namespace views {
namespace __chunk_by {
struct __fn {
template <class _Range, class _Pred>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Pred&& __pred) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Range&& __range, _Pred&& __pred)
noexcept(noexcept(/**/ chunk_by_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred))))
-> decltype(/*--*/ chunk_by_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred))) {
return /*-------------*/ chunk_by_view(std::forward<_Range>(__range), std::forward<_Pred>(__pred));
@@ -213,9 +213,9 @@ struct __fn {
template <class _Pred>
requires constructible_from<decay_t<_Pred>, _Pred>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Pred&& __pred)
noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) {
- return __range_adaptor_closure_t(std::__bind_back(*this, std::forward<_Pred>(__pred)));
+ return __range_adaptor_closure_t(std::__bind_back(operator(), std::forward<_Pred>(__pred)));
}
};
} // namespace __chunk_by
diff --git a/libcxx/include/__ranges/repeat_view.h b/libcxx/include/__ranges/repeat_view.h
index 620a2645497285..5caea757a39314 100644
--- a/libcxx/include/__ranges/repeat_view.h
+++ b/libcxx/include/__ranges/repeat_view.h
@@ -229,14 +229,13 @@ namespace views {
namespace __repeat {
struct __fn {
template <class _Tp>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __value) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Tp&& __value)
noexcept(noexcept(ranges::repeat_view(std::forward<_Tp>(__value))))
-> decltype( ranges::repeat_view(std::forward<_Tp>(__value)))
{ return ranges::repeat_view(std::forward<_Tp>(__value)); }
-
template <class _Tp, class _Bound>
- _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __value, _Bound&& __bound_sentinel) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Tp&& __value, _Bound&& __bound_sentinel)
noexcept(noexcept(ranges::repeat_view(std::forward<_Tp>(__value), std::forward<_Bound>(__bound_sentinel))))
-> decltype( ranges::repeat_view(std::forward<_Tp>(__value), std::forward<_Bound>(__bound_sentinel)))
{ return ranges::repeat_view(std::forward<_Tp>(__value), std::forward<_Bound>(__bound_sentinel)); }
diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h
index cf162100ee46b7..67818c521b1500 100644
--- a/libcxx/include/__ranges/to.h
+++ b/libcxx/include/__ranges/to.h
@@ -207,7 +207,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto to(_Args&&... __args)
static_assert(
!is_volatile_v<_Container>, "The target container cannot be volatile-qualified, please remove the volatile");
- auto __to_func = []<input_range _Range, class... _Tail>(_Range&& __range, _Tail&&... __tail)
+ auto __to_func = []<input_range _Range, class... _Tail>(_Range&& __range, _Tail&&... __tail) static
requires requires { //
/**/ ranges::to<_Container>(std::forward<_Range>(__range), std::forward<_Tail>(__tail)...);
}
@@ -223,7 +223,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto to(_Args&&... __args)
// clang-format off
auto __to_func = []<input_range _Range, class... _Tail,
class _DeducedExpr = typename _Deducer<_Container, _Range, _Tail...>::type>
- (_Range&& __range, _Tail&& ... __tail)
+ (_Range&& __range, _Tail&& ... __tail) static
requires requires { //
/**/ ranges::to<_DeducedExpr>(std::forward<_Range>(__range), std::forward<_Tail>(__tail)...);
}
diff --git a/libcxx/include/__ranges/zip_view.h b/libcxx/include/__ranges/zip_view.h
index ce00a4e53a489b..4ded5bca550341 100644
--- a/libcxx/include/__ranges/zip_view.h
+++ b/libcxx/include/__ranges/zip_view.h
@@ -489,10 +489,10 @@ namespace views {
namespace __zip {
struct __fn {
- _LIBCPP_HIDE_FROM_ABI constexpr auto operator()() const noexcept { return empty_view<tuple<>>{}; }
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()() noexcept { return empty_view<tuple<>>{}; }
template <class... _Ranges>
- _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Ranges&&... __rs) const
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto operator()(_Ranges&&... __rs)
noexcept(noexcept(zip_view<all_t<_Ranges&&>...>(std::forward<_Ranges>(__rs)...)))
-> decltype(zip_view<all_t<_Ranges&&>...>(std::forward<_Ranges>(__rs)...)) {
return zip_view<all_t<_Ranges>...>(std::forward<_Ranges>(__rs)...);
|
xiaoyang-sde
changed the title
static operator() for C++23 ranges
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const | ||
| noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| return __range_adaptor_closure_t(std::__bind_back(*this, std::forward<_Pred>(__pred))); | ||
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto | ||
| operator()(_Pred&& __pred) noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| constexpr auto __self = __fn{}; | ||
| return __range_adaptor_closure_t(std::__bind_back(__self, std::forward<_Pred>(__pred))); |
There was a problem hiding this comment.
std::__bind_back(*this, std::forward<_Pred>(__pred))won't compile becausethisis not available in astaticfunction.std::__bind_back(operator(), std::forward<_Pred>(__pred))won't compile because the compiler can't determine which overload ofoperator()to use.
The solution here is to initialize the function object __fn as a constexpr variable, which will be evaluated at compile time, and pass it to std::__bind_back. Please let me know if there's a better solution!
There was a problem hiding this comment.
The solution here is to initialize the function object
__fnas aconstexprvariable, which will be evaluated at compile time, and pass it tostd::__bind_back. Please let me know if there's a better solution!
Perhaps we can "pass" the functor as a template argument in the same way as P2714R1 (adopted for C++26).
There was a problem hiding this comment.
I wonder whether we should change this place. We need a *this pointer so what is the benefit of this change?
There was a problem hiding this comment.
I wonder whether we should change this place. We need a
*thispointer so what is the benefit of this change?
This change allows the first overload of operator() to be static. We just need to figure out how to pass __fn to std::__bind_back here.
If P2714R1 is implemented, I think we can write it like this: (credit @frederick-vs-ja)
- constexpr auto __self = __fn{};
- return __range_adaptor_closure_t(std::__bind_back(__self, std::forward<_Pred>(__pred)));
+ return __range_adaptor_closure_t(std::__bind_back<__fn{}>(std::forward<_Pred>(__pred)));There was a problem hiding this comment.
We can't this is a C++23 feature and P2714 is a C++26 feature.
There was a problem hiding this comment.
@philnik777 @frederick-vs-ja What do you think?
Context: philnik777 has initiated this pull requset and frederick-vs-ja has implemented a similar change on Microsoft STL (microsoft/STL#4358)
There was a problem hiding this comment.
If P2714R1 is implemented
We can't this is a C++23 feature and P2714 is a C++26 feature.
Oh, but IIUC we don't need to be waiting for P2714R1 being implemented. __bind_back is not a standard feature and it seems OK to make it equivalent to C++26 std::bind_back in C++20 mode.
There was a problem hiding this comment.
The __bind_back implementation detail is used C++20 so that "runs" into the same issue. I really want to avoid this change in this PR. We can make a separate PR where we add a __bind_back for C++23 which does that. However in that case I want to see a measurable performance improvement before adding that.
There was a problem hiding this comment.
The
__bind_backimplementation detail is used C++20 so that "runs" into the same issue. I really want to avoid this change in this PR. We can make a separate PR where we add a__bind_backfor C++23 which does that. However in that case I want to see a measurable performance improvement before adding that.
Sounds good to me. Thanks for reviewing this PR!
There was a problem hiding this comment.
The changes on this file have been reverted.
|
Once this pull request is merged, it's worth considering backporting this feature to those related to C++20 ranges, given that both Clang and GCC have implemented language extensions that support |
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const | ||
| noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| return __range_adaptor_closure_t(std::__bind_back(*this, std::forward<_Pred>(__pred))); | ||
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto | ||
| operator()(_Pred&& __pred) noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| constexpr auto __self = __fn{}; | ||
| return __range_adaptor_closure_t(std::__bind_back(__self, std::forward<_Pred>(__pred))); |
There was a problem hiding this comment.
I wonder whether we should change this place. We need a *this pointer so what is the benefit of this change?
There was a problem hiding this comment.
Once this pull request is merged, it's worth considering backporting this feature to those related to C++20 ranges, given that both Clang and GCC have implemented language extensions that support
static operator()in C++20. I'm willing to submit a pull request if that's a good idea.
I'm strongly against that idea. We agreed to allowed a small set of extensions to be used in older language versions to make the maintenance between language versions easier. We explicitly did not allow to use all extensions.
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Pred&& __pred) const | ||
| noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| return __range_adaptor_closure_t(std::__bind_back(*this, std::forward<_Pred>(__pred))); | ||
| _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI static constexpr auto | ||
| operator()(_Pred&& __pred) noexcept(is_nothrow_constructible_v<decay_t<_Pred>, _Pred>) { | ||
| constexpr auto __self = __fn{}; | ||
| return __range_adaptor_closure_t(std::__bind_back(__self, std::forward<_Pred>(__pred))); |
There was a problem hiding this comment.
We can't this is a C++23 feature and P2714 is a C++26 feature.
That was ever part of the reasoning. It's to reduce maintenance burden in general and possibly improve the QoI. IMO this clearly falls under "improve the QoI".
Yes. But that doesn't prevent us from allowing more useful extensions, such as this one. |
I strongly disagree with this statement, especially the "clearly" part. I have not seen any numbers proving this change has a measurable benefit on code size or performance. Without these I don't know whether this will be a measurable improvement and when it is not how much.
It does not per se, but this is what we discussed and agreed upon. We didn't want to blanket allow all possible extensions. |
Then feel free to look at the motivation for introducing the feature, which is referenced in the commit message from this very PR. |
|
✅ With the latest revision this PR passed the Python code formatter. |
Thanks for approving this PR! Could you please merge it once all CI checks have been passed? |
Abstract
This pull request converts the
operator()of all CPOs and niebloids related to C++23 ranges tostatic.Motivation
In
libc++, CPOs and niebloids are implemented as function objects. Currently, theoperator()for such a function object is aconst-qualified member function. This means that even if the function object is has no data members, an extra register is used to pass in thethispointer when callingoperator(), unless the compiler can inline the function call. Declaraingoperator()asstaticwould optimize away the unnecessarythispointer passing for stateless function objects, since there is no object instance state that needs to be accessed.Reference
operator()