Skip to content

GH-50713: [C++] Replace return_type and enable_if_return with <type_traits> helpers - #50714

Merged
pitrou merged 2 commits into
apache:mainfrom
taepper:minimize-functional-metaprogramming
Jul 30, 2026
Merged

GH-50713: [C++] Replace return_type and enable_if_return with <type_traits> helpers#50714
pitrou merged 2 commits into
apache:mainfrom
taepper:minimize-functional-metaprogramming

Conversation

@taepper

@taeppertaepper commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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_type related helpers from functional.h. Also, the unused helpers is_overloaded, enable_if_empty and enable_if_not_empty are removed.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@taepper
taepper requested a review from pitrou as a code ownerJuly 29, 2026 15:13
@github-actionsgithub-actionsBot added the awaiting review Awaiting review label Jul 29, 2026
@github-actions

Copy link
Copy Markdown

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

@github-actions
github-actionsBot marked this pull request as draft July 29, 2026 15:16
@taeppertaepper changed the title [GH-50713] replace return_type and enable_if_return with <type_traits> helpersGH-50713: [C++] replace return_type and enable_if_return with <type_traits> helpersJul 29, 2026
@taepper
taepper marked this pull request as ready for review July 29, 2026 15:17
@taepper

taepper commented Jul 29, 2026

Copy link
Copy Markdown
ContributorAuthor

@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>>
voidVisitGroupedValues(const ExecSpan& batch, ConsumeValue&& valid_func,
ConsumeNull&& null_func) {

I used typename GetViewType<Type>::T to e.g. map from Int32Type to int32_t / Decimal128Type to Decimal128. Is this the idiomatic way (we are not using a View here)? I see that also UnboxScalar<Type>::T might be possible?

@pitrou

Copy link
Copy Markdown
Member

Yes, this seems ok to me.

@pitrou

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 29, 2026
@github-actions

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>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And FYI, C++20 provides std::same_as to replace std::is_same_v.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment threadcpp/src/arrow/util/iterator.h Outdated
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<

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The role of Enable is to trigger SFINAE, so we could replace it with requires.

@taeppertaepperJul 29, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to push these changes @pitrou ?

Yes, please do.

@HuaHuaYHuaHuaY left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR. I've left two comments. We can consider whether to incorporate them in the future PRs. To be honest, I’m not sure how much syntax our CI can handle.

@pitrou

Copy link
Copy Markdown
Member

The failing C++ builds above are unrelated, FTR.

@taepper
taepper requested a review from pitrouJuly 30, 2026 11:35
@pitroupitrou added the CI: Extra: C++ Run extra C++ CI label Jul 30, 2026

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I'll wait for the CI builds

@taepper

Copy link
Copy Markdown
ContributorAuthor

I guess I should rebase onto main given the CI failures mentioning simdjson which should have been fixed in #50732

@taepper
taepperforce-pushed the minimize-functional-metaprogramming branch from f07ffc9 to 42bbe09CompareJuly 30, 2026 13:47
@pitrou

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@pitrou

Copy link
Copy Markdown
Member

I'll wait for the latest CI results and then merge :)

@github-actions

Copy link
Copy Markdown

Revision: 42bbe09

Submitted crossbow builds: ursacomputing/crossbow @ actions-e1b94b023a

TaskStatus
example-cpp-minimal-build-staticGitHub Actions
example-cpp-minimal-build-static-system-dependencyGitHub Actions
example-cpp-tutorialGitHub Actions
test-build-cpp-fuzzGitHub Actions
test-conda-cppGitHub Actions
test-conda-cpp-valgrindGitHub Actions
test-debian-13-cpp-amd64GitHub Actions
test-debian-13-cpp-i386GitHub Actions
test-debian-experimental-cpp-gcc-15GitHub Actions
test-fedora-42-cppGitHub Actions
test-ubuntu-22.04-cppGitHub Actions
test-ubuntu-22.04-cpp-bundledGitHub Actions
test-ubuntu-22.04-cpp-emscriptenGitHub Actions
test-ubuntu-22.04-cpp-no-threadingGitHub Actions
test-ubuntu-24.04-cppGitHub Actions
test-ubuntu-24.04-cpp-bundled-offlineGitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundledGitHub Actions
test-ubuntu-24.04-cpp-gcc-14GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formatsGitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizerGitHub Actions

@pitroupitrou changed the title GH-50713: [C++] replace return_type and enable_if_return with <type_traits> helpersGH-50713: [C++] Replace return_type and enable_if_return with <type_traits> helpersJul 30, 2026
@pitrou
pitrou merged commit 5d6cb93 into apache:mainJul 30, 2026
63 of 67 checks passed
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5d6cb93.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@taepper@pitrou@HuaHuaY