GH-50713: [C++] Replace return_type and enable_if_return with <type_traits> helpers - #50714
Conversation
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
return_type and enable_if_return with <type_traits> helpersreturn_type and enable_if_return with <type_traits> helpers
|
@pitrou For this case: template <typename Type, typename ConsumeValue, typename ConsumeNull>
requires std::is_void_v<
std::invoke_result_t<ConsumeValue, uint32_t, typename GetViewType<Type>::T>>
void VisitGroupedValues(const ExecSpan& batch, ConsumeValue&& valid_func,
ConsumeNull&& null_func) {I used |
|
Yes, this seems ok to me. |
|
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
| VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func, | ||
| NullFunc&& null_func) { | ||
| requires std::is_same_v<std::invoke_result_t<VisitFunc, typename GetViewType<T>::T>, | ||
| Status> |
There was a problem hiding this comment.
I think we may be able to use std::is_invocable_r_v here. It might also work for cases where the return type is void. It depends on how strict we want the constraints to be.
There was a problem hiding this comment.
And FYI, C++20 provides std::same_as to replace std::is_same_v.
There was a problem hiding this comment.
And FYI, C++20 provides std::same_as to replace std::is_same_v.
Ah, nice! I added #50720 for this change, as this is not widely used in the code-base for now.
I think we may be able to use std::is_invocable_r_v here. It might also work for cases where the return type is void. It depends on how strict we want the constraints to be.
I am not sure whether I find the std::is_invocable_r_v more readable in this codebase, compared to using more well-known constructs?
| template <typename Fn, typename From, | ||
| typename Ret = typename std::invoke_result_t<Fn&, From>::ValueType, | ||
| typename To = std::tuple_element_t<0, Ret>, | ||
| typename Enable = std::enable_if_t< |
There was a problem hiding this comment.
The role of Enable is to trigger SFINAE, so we could replace it with requires.
There was a problem hiding this comment.
This is a very good point, I fixed this locally:
commit f07ffc993ea0337052490b5fedc8d5ea4c601b5a (HEAD -> minimize-functional-metaprogramming)
Author: Alexander Taepper <alexander.taepper@gmail.com>
Date: Wed Jul 29 19:05:30 2026 +0200
replace another `typename Enable` with `requires`
diff --git a/cpp/src/arrow/util/iterator.h b/cpp/src/arrow/util/iterator.h
index 3a098b1455..aaf6531dfe 100644
--- a/cpp/src/arrow/util/iterator.h
+++ b/cpp/src/arrow/util/iterator.h
@@ -519,9 +519,8 @@ struct FilterIterator {
/// \brief Like MapIterator, but where the function can fail or reject elements.
template <typename Fn, typename From,
typename Ret = typename std::invoke_result_t<Fn&, From>::ValueType,
- typename To = std::tuple_element_t<0, Ret>,
- typename Enable = std::enable_if_t<
- std::is_same_v<std::tuple_element_t<1, Ret>, FilterIterator::Action>>>
+ typename To = std::tuple_element_t<0, Ret>>
+ requires std::is_same_v<std::tuple_element_t<1, Ret>, FilterIterator::Action>
Iterator<To> MakeFilterIterator(Fn filter, Iterator<From> it) {
return Iterator<To>(
FilterIterator::Impl<Fn, From, To>(std::move(filter), std::move(it)));Is it okay to push these changes @pitrou ? I can also do this in the second part of #50250 instead.
There was a problem hiding this comment.
Is it okay to push these changes @pitrou ?
Yes, please do.
|
The failing C++ builds above are unrelated, FTR. |
pitrou
left a comment
There was a problem hiding this comment.
+1, I'll wait for the CI builds
|
I guess I should rebase onto |
f07ffc9 to
42bbe09
Compare
|
@github-actions crossbow submit -g cpp |
|
I'll wait for the latest CI results and then merge :) |
|
Revision: 42bbe09 Submitted crossbow builds: ursacomputing/crossbow @ actions-e1b94b023a |
return_type and enable_if_return with <type_traits> helpersreturn_type and enable_if_return with <type_traits> helpers
Rationale for this change
This is the first part of simplifying functional helpers which are no longer required since the code-base supports more recent C++ versions. (See #50713 and #50250)
What changes are included in this PR?
This removes the
return_typerelated helpers fromfunctional.h. Also, the unused helpersis_overloaded,enable_if_emptyandenable_if_not_emptyare removed.Are these changes tested?
Yes
Are there any user-facing changes?
No
return_typehelpers fromutil/functional.h#50713