seastar
seastar copied to clipboard
rpc: cast rpc::tuple to std::tuple when passing it to std::apply()
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
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.
@scylladb/seastar-maint hello maintainers, could you help review this change?
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.
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
.
The context is here e51a1a8ed955030774cf3ae406d725613e4a099b
@gleb-cloudius thank you Gleb. seems i need to implement a std::apply()
which works with rpc::tuple
.
But if rpc::tuple is just an alias and not a separate type how can it be implemented?
@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.
@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 ownapply()
which does not enforce the type constraints.
Ah. I misunderstood. Sounds fine.
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
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++.
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.
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.
@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?
@nyh could you please take another look?
@scylladb/seastar-maint hello maintainers, could you help merge this change?
@scylladb/seastar-maint hello maintainers, could you help merge this change?
v3:
- instead of duplicating
std::apply
, just castrpc::tuple
tostd::tuple
.
@avikivity thank you for your suggestion. incorporated and it works great. could you take another look?
@avikivity could you take another look?