seastar icon indicating copy to clipboard operation
seastar copied to clipboard

rpc: cast rpc::tuple to std::tuple when passing it to std::apply()

Open tchaikov opened this issue 10 months ago • 19 comments

before this change, we implement rpc::tuple by inheriting it from std::tuple, and following the tuple-like protocol, by implementing tuple_size<seastar::rpc::tuple<T...>> and tuple_element<I,seastar::rpc::tuple<T...>>. but in C++23, the tuple-like constraints are tightened, quote from libstdc++ shipped along with GCC-14

  template<typename _Tp>
    inline constexpr bool __is_tuple_v = false;

  template<typename... _Ts>
    inline constexpr bool __is_tuple_v<tuple<_Ts...>> = true;

  // TODO: Reuse __is_tuple_like from <type_traits>?
  template<typename _Tp>
    inline constexpr bool __is_tuple_like_v = false;

  template<typename... _Elements>
    inline constexpr bool __is_tuple_like_v<tuple<_Elements...>> = true;

  template<typename _T1, typename _T2>
    inline constexpr bool __is_tuple_like_v<pair<_T1, _T2>> = true;

  template<typename _Tp, size_t _Nm>
    inline constexpr bool __is_tuple_like_v<array<_Tp, _Nm>> = true;
// ...

which is a loyal translation of the C++23 standard.

apparently, rpc::tuple cannot fulfill this constraint, hence rpc_test.cc fails to compile with GCC-14 and C++23. fortunately, rpc::tuple is derived from std::tuple.

so, in this change, we cast rpc::tuple to std::tuple when passing it to std::apply to satisfy the more picky critera in C++23.

Fixes https://github.com/scylladb/seastar/issues/2158

tchaikov avatar Mar 27 '24 12:03 tchaikov

to be more future-proof and standard compliant, either we need to avoid using std::apply() or to deprecate the non-alias version along with its deduction guide.

tchaikov avatar Mar 27 '24 13:03 tchaikov

@scylladb/seastar-maint hello maintainers, could you help review this change?

tchaikov avatar Apr 03 '24 02:04 tchaikov

Before I merge this, @gleb-cloudius can you please review this PR too? I think you have more context than me why we even introduced this "rpc::tuple" thing and didn't just use std::tuple directly.

nyh avatar Apr 03 '24 07:04 nyh

thank you for your approval, Nadav. my guess is that the deduction guide cannot be applied to a template alias. but it's handy to use -- make_tuple is shorter than tuple.

tchaikov avatar Apr 03 '24 07:04 tchaikov

The context is here e51a1a8ed955030774cf3ae406d725613e4a099b

gleb-cloudius avatar Apr 03 '24 07:04 gleb-cloudius

@gleb-cloudius thank you Gleb. seems i need to implement a std::apply() which works with rpc::tuple.

tchaikov avatar Apr 03 '24 07:04 tchaikov

But if rpc::tuple is just an alias and not a separate type how can it be implemented?

gleb-cloudius avatar Apr 03 '24 07:04 gleb-cloudius

@gleb-cloudius i intend to respect the original idea

I chose to introduce rpc::tuple instead of using std::tuple, since there may be existing users of std::tuple in rpc, with its own serialization.

so, will keep it as a derived class of std::tuple. and instead implement our own apply() which does not enforce the type constraints.

tchaikov avatar Apr 03 '24 08:04 tchaikov

@gleb-cloudius i intend to respect the original idea

I chose to introduce rpc::tuple instead of using std::tuple, since there may be existing users of std::tuple in rpc, with its own serialization.

so, will keep it as a derived class of std::tuple. and instead implement our own apply() which does not enforce the type constraints.

Ah. I misunderstood. Sounds fine.

gleb-cloudius avatar Apr 03 '24 08:04 gleb-cloudius

tested with clang-19 + c++23.

$ grep CMAKE_CXX_STANDARD build/debug/CMakeCache.txt
CMAKE_CXX_STANDARD:STRING=23
$ cmake --build /var/ssd/seastar/build/debug --target test_unit_rpc
 [1/7] Building CXX object CMakeFiles/seastar.dir/src/rpc/lz4_fragmented_compressor.cc.o
 [2/7] Building CXX object CMakeFiles/seastar.dir/src/rpc/lz4_compressor.cc.o
 [3/7] Building CXX object CMakeFiles/seastar.dir/src/rpc/rpc.cc.o
 [4/7] Linking CXX shared library libseastar.so
 [5/7] Linking CXX shared library libseastar_testing.so
 [6/7] Building CXX object tests/unit/CMakeFiles/test_unit_rpc.dir/rpc_test.cc.o
 [7/7] Linking CXX executable tests/unit/rpc_test

tchaikov avatar Apr 03 '24 08:04 tchaikov

v2:

  • s/apply/rpc::apply/ to address the build failure like /home/circleci/project/include/seastar/rpc/rpc_impl.hh:261:13: error: call to 'apply' is ambiguous when compiling with older libstdc++.

tchaikov avatar Apr 03 '24 10:04 tchaikov

Alternatively: model tuple-like (https://en.cppreference.com/w/cpp/utility/tuple/tuple-like). These need to overload std::tuple_size, std::tuple_element, and std::get.

avikivity avatar Apr 03 '24 10:04 avikivity

Alternatively: model tuple-like (https://en.cppreference.com/w/cpp/utility/tuple/tuple-like). These need to overload std::tuple_size, std::tuple_element, and std::get.

@avikivity it's a no-go. we cannot model tuple-like in C++23 or at least with __cpp_lib_tuple_like. please take a closer look at the cover letter and the explanation in the link above actually renders it impossible. std::tuple_size, std::tuple_element, and std::get help with the structured-binding, but they do not get a homebrew tuple a ticket into the tuple-alike club.

tchaikov avatar Apr 03 '24 10:04 tchaikov

@nyh hi Nadav, per Gleb's comment, i am using another approach to address the build failure on C++23, could you take another look?

tchaikov avatar Apr 04 '24 03:04 tchaikov

@nyh could you please take another look?

tchaikov avatar Apr 16 '24 01:04 tchaikov

@scylladb/seastar-maint hello maintainers, could you help merge this change?

tchaikov avatar Apr 19 '24 00:04 tchaikov

@scylladb/seastar-maint hello maintainers, could you help merge this change?

tchaikov avatar Apr 30 '24 07:04 tchaikov

v3:

  • instead of duplicating std::apply, just cast rpc::tuple to std::tuple.

tchaikov avatar May 01 '24 02:05 tchaikov

@avikivity thank you for your suggestion. incorporated and it works great. could you take another look?

tchaikov avatar May 01 '24 02:05 tchaikov

@avikivity could you take another look?

tchaikov avatar May 06 '24 00:05 tchaikov