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

@taepper taepper 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 owner July 29, 2026 15:13
@github-actions github-actions Bot 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-actions Bot marked this pull request as draft July 29, 2026 15:16
@taepper taepper changed the title [GH-50713] replace return_type and enable_if_return with <type_traits> helpers GH-50713: [C++] replace return_type and enable_if_return with <type_traits> helpers Jul 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
Contributor Author

@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 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

pitrou commented Jul 29, 2026

Copy link
Copy Markdown
Member

Yes, this seems ok to me.

@pitrou

pitrou commented Jul 29, 2026

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actions github-actions Bot 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
Contributor Author

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 thread cpp/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.

@taepper taepper Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

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.

@HuaHuaY HuaHuaY 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

pitrou commented Jul 29, 2026

Copy link
Copy Markdown
Member

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

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

@pitrou pitrou 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

taepper commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

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

@taepper
taepper force-pushed the minimize-functional-metaprogramming branch from f07ffc9 to 42bbe09 Compare July 30, 2026 13:47
@pitrou

pitrou commented Jul 30, 2026

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@pitrou

pitrou commented Jul 30, 2026

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

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-debian-13-cpp-amd64 GitHub Actions
test-debian-13-cpp-i386 GitHub Actions
test-debian-experimental-cpp-gcc-15 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@pitrou pitrou changed the title GH-50713: [C++] replace return_type and enable_if_return with <type_traits> helpers GH-50713: [C++] Replace return_type and enable_if_return with <type_traits> helpers Jul 30, 2026
@pitrou
pitrou merged commit 5d6cb93 into apache:main Jul 30, 2026
63 of 67 checks passed
Sign up for free to 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