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.

@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 pushed a commit to Alex-PLACET/arrow that referenced this pull request Sep 17, 2026
… `<type_traits>` (apache#51328)

### Rationale for this change

This is the last change for apache#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

* GitHub Issue: apache#50250

Authored-by: Alexander Taepper <alexander.taepper@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
pitrou pushed a commit that referenced this pull request Nov 9, 2026
…/benchmarks (#51384)

Closes #51367

### Rationale for this change

No code within Arrow uses `arrow::internal::LruCache`. Per discussion in
#51328 (review comment r4027861697), removing this dead code along with
its tests and benchmarks.

### What changes are included in this PR?

- Removed cpp/src/arrow/util/cache_internal.h (LruCache, MemoizeLru,
  MemoizeLruThreadUnsafe — all unused outside this file and its tests)
- Removed cpp/src/arrow/util/cache_test.cc
- Removed cpp/src/arrow/util/cache_benchmark.cc
- Removed corresponding entries from cpp/src/arrow/util/CMakeLists.txt

Verified via `git grep` across cpp/ that nothing else references this
code (gandiva's separate LRU cache in cpp/src/gandiva/lru_cache.h is
unrelated and untouched).

Supersedes #51371, which was opened with an empty diff due to a branch mix-up.

### Are these changes tested?

Pure removal of unused code; no new tests needed. Relying on CI to
validate the build/test suite still passes.

### Are there any user-facing changes?

No.

### Was AI used for this PR?

**PR code and description written by:**

- [x] Human
- [x] AI

**Reviewed before submission by:**

- [x] Human
- [ ] AI
- [ ] Not reviewed

Authored-by: Basant_Jamwal <basantrajput570@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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