Skip to content

GH-50250: [C++] Remove call_traits::argument_type in favor of <type_traits> - #51328

Merged
pitrou merged 1 commit into
apache:mainfrom
taepper:remove-argument-type
Sep 16, 2026
Merged

pitrou merged 1 commit into
apache:mainfrom
taepper:remove-argument-type

Conversation

@taepper

@taepper taepper commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

This is the last change for #50250. The remaining FnOnce does not have corresponding utilities in the C++20 standard library according to my knowledge.

What changes are included in this PR?

This removes the custom meta-programming facility call_traits::argument_type. We instead use the provided type_traits header which allows us to check invocability (and thus indirectly the argument type) with e.g. std::is_invocable.

It needs to be mentioned that none of std::is_invocable_v, std::invoke_result_t, .. is a drop-in replacement for the removed argument_type. I instead migrated all users of the old facility to the standard constructs. Some call-sites needed changing by supplying template parameters explicitly, but in my opinion all changes are defendable or even improvements.

Are these changes tested?

Yes, this refactoring commit still passes all test cases

Are there any user-facing changes?

No

std::make_shared<Memoizer>(std::forward<Func>(func), cache_capacity)};

return shared_memoized;
return std::function<RetType(const Key&)>(std::move(shared_memoized));

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.

Wrapping the result in a std::function adds a layer of indirection when calling operator() that might be expensive. Can we avoid this?

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.

Maybe I am understanding the code wrong, but I thought the change is equivalent.

The previous version had the function declaration:
static std::function<RetType(const Key&)> Memoize(...)

whereas now we have
static auto Memoize(...)

so all I did in the return statement was changing the implicit conversion to a std::function to an explicit one

@HuaHuaY HuaHuaY Sep 16, 2026

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 previously copied and modified this file in iceberg-cpp repo. The file in iceberg-cpp might be as pitrou described. Maybe we can open a new PR to refactor the code. https://github.com/apache/iceberg-cpp/pull/891/changes#diff-6e67f7ceb80cef4c07b66e68bc6907d481231abde46ce1168ed492eadbceb6f1

template <template <typename...> class MemoizerType, typename Func>
auto Memoize(Func&& func, int32_t cache_capacity) {
  using Function = decltype(std::function{std::forward<Func>(func)});
  using Key = std::decay_t<typename unary_traits<Function>::arg>;
  using Value = std::decay_t<std::invoke_result_t<Func, const Key&>>;
  using Memoizer = MemoizerType<Key, Value, LruCache<Key, Value>, Func>;
  return Memoizer(std::forward<Func>(func), cache_capacity);
}

// Apply a LRU memoization cache to a callable.
template <typename Func>
auto MemoizeLru(Func&& func, int32_t cache_capacity) {
  return Memoize<ThreadSafeMemoizer>(std::forward<Func>(func), cache_capacity);
}

However, I discovered that no code within Arrow currently uses this arrow::internal::LruCache; perhaps we can simply delete the file.

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.

The previous version had the function declaration:
static std::function<RetType(const Key&)> Memoize(...)

Oh, you're right, I misread. Sorry for the noise!

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.

However, I discovered that no code within Arrow currently uses this arrow::internal::LruCache; perhaps we can simply delete the file.

Let's open a separate issue for that?

@pitrou

pitrou commented Sep 14, 2026

Copy link
Copy Markdown
Member

Unfortunately we'll have to wait for #51326 before we can ensure that this doesn't break on some C++ compilers on our CI platforms.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 14, 2026
@pitrou
pitrou force-pushed the remove-argument-type branch from 1075e31 to f7967e6 Compare September 16, 2026 15:39
@pitrou

pitrou commented Sep 16, 2026

Copy link
Copy Markdown
Member

I've rebased and hopefully we can get mostly green CI.

@pitrou
pitrou merged commit 6219555 into apache:main Sep 16, 2026
57 of 59 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants