From af816a4d99bc0d20e25148a5cc8903b935997495 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 1 Jul 2022 21:18:52 +0800 Subject: [PATCH 1/5] serde: use concepts instead of type traits * rewrite the type traits using concept for better readability. * reuse the is_envelope<> concept when appropriate less repeatings this way * drop unnecessary constraits detail::compat_version_has_serde_version_type implies detail::has_compat_attribute, so we can drop the latter. detail::version_has_serde_version_type implies implies detail::has_version_attribute, so we can drop the latter. * drop helper concepts and inline them they are not used elsewhere, so better off inlining them. * replace std::decay_t with same_as, shorter this way. this helps us to drop the `#clang-format off` guard. * check value of tags. as they should be true. Signed-off-by: Kefu Chai --- src/v/serde/envelope.h | 75 +++------------------------ src/v/serde/envelope_for_each_field.h | 8 +-- src/v/serde/serde.h | 16 +++--- src/v/serde/test/fuzz.cc | 9 +--- src/v/serde/test/serde_test.cc | 12 ++--- 5 files changed, 28 insertions(+), 92 deletions(-) diff --git a/src/v/serde/envelope.h b/src/v/serde/envelope.h index 83c8d24f606d..c4c02a960c04 100644 --- a/src/v/serde/envelope.h +++ b/src/v/serde/envelope.h @@ -10,6 +10,7 @@ #pragma once #include +#include #include namespace serde { @@ -67,77 +68,17 @@ struct checksum_envelope { static constexpr auto redpanda_serde_build_checksum = true; }; -namespace detail { - -template -struct has_compat_attribute : std::false_type {}; - -template -struct has_compat_attribute< - T, - std::void_t().redpanda_serde_compat_version)>> - : std::true_type {}; - -template -struct has_version_attribute : std::false_type {}; - -template -struct has_version_attribute< - T, - std::void_t().redpanda_serde_version)>> - : std::true_type {}; - -template -struct inherits_from_envelope : std::false_type {}; - -template -struct inherits_from_envelope< - T, - std::void_t().redpanda_inherits_from_envelope)>> - : std::true_type {}; - -template -struct compat_version_has_serde_version_type { - static constexpr auto const value = std::is_same_v< - std::decay_t().redpanda_serde_compat_version)>, - version_t>; +template +concept is_envelope = requires { + { T::redpanda_serde_version } -> std::same_as; + { T::redpanda_serde_compat_version } -> std::same_as; }; template -struct version_has_serde_version_type { - static constexpr auto const value = std::is_same_v< - std::decay_t().redpanda_serde_version)>, - version_t>; -}; - -template -struct has_checksum_attribute : std::false_type {}; - -template -struct has_checksum_attribute< - T, - std::void_t().redpanda_serde_build_checksum)>> - : std::true_type {}; - -} // namespace detail - -template -inline constexpr auto const is_envelope_v = std::conjunction_v< - detail::has_compat_attribute, - detail::has_version_attribute, - detail::compat_version_has_serde_version_type, - detail::version_has_serde_version_type>; - -template -inline constexpr auto const is_checksum_envelope_v = std::conjunction_v< - detail::has_compat_attribute, - detail::has_version_attribute, - detail::compat_version_has_serde_version_type, - detail::version_has_serde_version_type, - detail::has_checksum_attribute>; +concept is_checksum_envelope + = is_envelope && T::redpanda_serde_build_checksum; template -inline constexpr auto const inherits_from_envelope_v - = detail::inherits_from_envelope::value; +concept inherits_from_envelope = T::redpanda_inherits_from_envelope; } // namespace serde diff --git a/src/v/serde/envelope_for_each_field.h b/src/v/serde/envelope_for_each_field.h index 772c4f12397f..20e452fd33e3 100644 --- a/src/v/serde/envelope_for_each_field.h +++ b/src/v/serde/envelope_for_each_field.h @@ -215,8 +215,8 @@ constexpr inline auto envelope_to_tuple(T& t) { template inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< !std::is_convertible_v())), bool>> { - static_assert(is_envelope_v>); - if constexpr (inherits_from_envelope_v>) { + static_assert(is_envelope>); + if constexpr (inherits_from_envelope>) { std::apply( [&](auto&&... args) { (fn(args), ...); }, envelope_to_tuple(t)); } else { @@ -228,8 +228,8 @@ inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< template inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< std::is_convertible_v())), bool>> { - static_assert(is_envelope_v>); - if constexpr (inherits_from_envelope_v>) { + static_assert(is_envelope>); + if constexpr (inherits_from_envelope>) { std::apply( [&](auto&&... args) { (void)(fn(args) && ...); }, envelope_to_tuple(t)); diff --git a/src/v/serde/serde.h b/src/v/serde/serde.h index 4174ef5a5347..015ca7e0b2f0 100644 --- a/src/v/serde/serde.h +++ b/src/v/serde/serde.h @@ -159,7 +159,7 @@ inline constexpr bool is_absl_node_hash_map_v = is_absl_node_hash_map::value; template inline constexpr auto const is_serde_compatible_v - = is_envelope_v + = is_envelope || (std::is_scalar_v // && (!std::is_same_v || std::numeric_limits::is_iec559) && (!std::is_same_v || std::numeric_limits::is_iec559) @@ -190,14 +190,14 @@ void write(iobuf& out, T t) { static_assert(are_bytes_and_string_different); static_assert(has_serde_write || is_serde_compatible_v); - if constexpr (is_envelope_v) { + if constexpr (is_envelope) { write(out, Type::redpanda_serde_version); write(out, Type::redpanda_serde_compat_version); auto size_placeholder = out.reserve(sizeof(serde_size_t)); auto checksum_placeholder = iobuf::placeholder{}; - if constexpr (is_checksum_envelope_v) { + if constexpr (is_checksum_envelope) { checksum_placeholder = out.reserve(sizeof(checksum_t)); } @@ -218,7 +218,7 @@ void write(iobuf& out, T t) { size_placeholder.write( reinterpret_cast(&size), sizeof(serde_size_t)); - if constexpr (is_checksum_envelope_v) { + if constexpr (is_checksum_envelope) { auto crc = crc::crc32c{}; auto in = iobuf_const_parser{out}; in.skip(size_before); @@ -388,7 +388,7 @@ header read_header(iobuf_parser& in, std::size_t const bytes_left_limit) { auto const size = read_nested(in, bytes_left_limit); auto checksum = checksum_t{}; - if constexpr (is_checksum_envelope_v) { + if constexpr (is_checksum_envelope) { checksum = read_nested(in, bytes_left_limit); } @@ -443,10 +443,10 @@ void read_nested(iobuf_parser& in, T& t, std::size_t const bytes_left_limit) { static_assert(are_bytes_and_string_different); static_assert(has_serde_read || is_serde_compatible_v); - if constexpr (is_envelope_v) { + if constexpr (is_envelope) { auto const h = read_header(in, bytes_left_limit); - if constexpr (is_checksum_envelope_v) { + if constexpr (is_checksum_envelope) { auto const shared = in.share(in.bytes_left() - h._bytes_left_limit); auto read_only_in = iobuf_const_parser{shared}; auto crc = crc::crc32c{}; @@ -695,7 +695,7 @@ ss::future> read_async(iobuf_parser& in) { template ss::future<> write_async(iobuf& out, T const& t) { using Type = std::decay_t; - if constexpr (is_envelope_v && has_serde_async_write) { + if constexpr (is_envelope && has_serde_async_write) { write(out, Type::redpanda_serde_version); write(out, Type::redpanda_serde_compat_version); diff --git a/src/v/serde/test/fuzz.cc b/src/v/serde/test/fuzz.cc index 91840047863a..115dc184a9cf 100644 --- a/src/v/serde/test/fuzz.cc +++ b/src/v/serde/test/fuzz.cc @@ -18,12 +18,7 @@ bool eq( return ((std::get(a) == std::get(b)) && ...); } -template< - typename T1, - typename T2, - typename std::enable_if_t< - serde::is_envelope_v && serde::is_envelope_v, - void*> = nullptr> +template bool operator==(T1 const& a, T2 const& b) { return eq( envelope_to_tuple(a), @@ -69,7 +64,7 @@ void init( data_gen& gen, std::index_sequence generations, int depth = 0) { - if constexpr (serde::is_envelope_v) { + if constexpr (serde::is_envelope) { ((std::apply( [&](auto&&... args) { (init(args, gen, generations, depth + 1), ...); diff --git a/src/v/serde/test/serde_test.cc b/src/v/serde/test/serde_test.cc index a631f6c672ce..f8ce5abf5070 100644 --- a/src/v/serde/test/serde_test.cc +++ b/src/v/serde/test/serde_test.cc @@ -90,10 +90,10 @@ struct test_msg1_new_manual { }; struct not_an_envelope {}; -static_assert(!serde::is_envelope_v); -static_assert(serde::is_envelope_v); -static_assert(serde::inherits_from_envelope_v); -static_assert(!serde::inherits_from_envelope_v); +static_assert(!serde::is_envelope); +static_assert(serde::is_envelope); +static_assert(serde::inherits_from_envelope); +static_assert(!serde::inherits_from_envelope); static_assert(test_msg1::redpanda_serde_version == 4); static_assert(test_msg1::redpanda_serde_compat_version == 0); @@ -234,7 +234,7 @@ struct complex_msg : serde::envelope> { int32_t _x; }; -static_assert(serde::is_envelope_v); +static_assert(serde::is_envelope); SEASTAR_THREAD_TEST_CASE(complex_msg_test) { auto b = iobuf(); @@ -386,7 +386,7 @@ struct test_snapshot_header int32_t metadata_size; }; -static_assert(serde::is_envelope_v); +static_assert(serde::is_envelope); static_assert(serde::has_serde_async_read); static_assert(serde::has_serde_async_write); From aa0f1d654b075cf286f84a440a7929d7c5f59c70 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 1 Jul 2022 21:42:33 +0800 Subject: [PATCH 2/5] serde: convert has_serde_fields_v to a concept more readable this way. Signed-off-by: Kefu Chai --- src/v/serde/envelope_for_each_field.h | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/v/serde/envelope_for_each_field.h b/src/v/serde/envelope_for_each_field.h index 20e452fd33e3..ba179b89287a 100644 --- a/src/v/serde/envelope_for_each_field.h +++ b/src/v/serde/envelope_for_each_field.h @@ -19,31 +19,21 @@ namespace serde { namespace detail { -template -struct has_serde_fields : std::false_type {}; - -template -struct has_serde_fields< - T, - std::void_t>().serde_fields())>> - : std::true_type {}; - template -inline constexpr auto const has_serde_fields_v = has_serde_fields::value; +concept has_serde_fields = requires(T t) { + t.serde_fields(); +}; } // namespace detail -template< - typename T, - std::enable_if_t, void*> = nullptr> +template constexpr inline auto envelope_to_tuple(T&& t) { return t.serde_fields(); } -template< - typename T, - std::enable_if_t, void*> = nullptr> -constexpr inline auto envelope_to_tuple(T& t) { +template +requires(!detail::has_serde_fields) constexpr inline auto envelope_to_tuple( + T& t) { static_assert(std::is_aggregate_v); static_assert(std::is_standard_layout_v); static_assert(!std::is_polymorphic_v); From 6eba9cab06a6b242a7aaee8e419c4870e58f4dce Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Wed, 6 Jul 2022 20:51:25 +0800 Subject: [PATCH 3/5] serde: add check_for_more_fn concept instead using `std::enable_if_t<>`, define a named concept for the type constraint of the function parameter. better readability this way. Signed-off-by: Kefu Chai --- src/v/serde/envelope_for_each_field.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/v/serde/envelope_for_each_field.h b/src/v/serde/envelope_for_each_field.h index ba179b89287a..2fd696c96e2d 100644 --- a/src/v/serde/envelope_for_each_field.h +++ b/src/v/serde/envelope_for_each_field.h @@ -202,10 +202,13 @@ requires(!detail::has_serde_fields) constexpr inline auto envelope_to_tuple( } } -template -inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< - !std::is_convertible_v())), bool>> { - static_assert(is_envelope>); +template +concept check_for_more_fn = requires(Fn&& fn, int& f) { + { fn(f) } -> std::convertible_to; +}; + +template +inline auto envelope_for_each_field(T& t, Fn&& fn) { if constexpr (inherits_from_envelope>) { std::apply( [&](auto&&... args) { (fn(args), ...); }, envelope_to_tuple(t)); @@ -215,10 +218,8 @@ inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< } } -template -inline auto envelope_for_each_field(T& t, Fn&& fn) -> std::enable_if_t< - std::is_convertible_v())), bool>> { - static_assert(is_envelope>); +template +inline auto envelope_for_each_field(T& t, Fn&& fn) { if constexpr (inherits_from_envelope>) { std::apply( [&](auto&&... args) { (void)(fn(args) && ...); }, From 131122c1a691fdcda303984b53e910d711c87b29 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Wed, 6 Jul 2022 20:52:26 +0800 Subject: [PATCH 4/5] serde: rewrite has_serde_async_{read,write} using concepts for better readability Signed-off-by: Kefu Chai --- src/v/serde/serde.h | 57 +++++++++++---------------------------------- 1 file changed, 13 insertions(+), 44 deletions(-) diff --git a/src/v/serde/serde.h b/src/v/serde/serde.h index 015ca7e0b2f0..8cc351cbe599 100644 --- a/src/v/serde/serde.h +++ b/src/v/serde/serde.h @@ -69,57 +69,26 @@ struct header { checksum_t _checksum; }; -template -struct help_has_serde_read : std::false_type {}; - -template -struct help_has_serde_read< - T, - std::void_t().serde_read( - std::declval>(), - std::declval
()))>> : std::true_type {}; - -template -inline constexpr auto const has_serde_read = help_has_serde_read::value; - -template -struct help_has_serde_write : std::false_type {}; - -template -struct help_has_serde_write< - T, - std::void_t().serde_write( - std::declval>()))>> : std::true_type {}; - template -inline constexpr auto const has_serde_write = help_has_serde_write::value; - -template -struct help_has_serde_async_read : std::false_type {}; - -template -struct help_has_serde_async_read< - T, - std::void_t().serde_async_read( - std::declval>(), - std::declval
()))>> : std::true_type {}; +concept has_serde_read = requires(T t, iobuf_parser& in, const header& h) { + t.serde_read(in, h); +}; template -inline constexpr auto const has_serde_async_read - = help_has_serde_async_read::value; - -template -struct help_has_serde_async_write : std::false_type {}; +concept has_serde_write = requires(T t, iobuf& out) { + t.serde_write(out); +}; template -struct help_has_serde_async_write< - T, - std::void_t().serde_async_write( - std::declval>()))>> : std::true_type {}; +concept has_serde_async_read + = requires(T t, iobuf_parser& in, const header& h) { + t.serde_async_read(in, h); +}; template -inline constexpr auto const has_serde_async_write - = help_has_serde_async_write::value; +concept has_serde_async_write = requires(T t, iobuf& out) { + t.serde_async_write(out); +}; using serde_enum_serialized_t = int32_t; From 25c31de8421340dcbf95f1d489b1e1d6fb2f8892 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 4 Jul 2022 21:52:23 +0800 Subject: [PATCH 5/5] serde: make has_serde_async_{read,write} more strict ensure that they return future<>. Signed-off-by: Kefu Chai --- src/v/serde/serde.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/v/serde/serde.h b/src/v/serde/serde.h index 8cc351cbe599..4bbfca2e017a 100644 --- a/src/v/serde/serde.h +++ b/src/v/serde/serde.h @@ -25,6 +25,7 @@ #include "utils/named_type.h" #include "vlog.h" +#include #include #include @@ -82,12 +83,12 @@ concept has_serde_write = requires(T t, iobuf& out) { template concept has_serde_async_read = requires(T t, iobuf_parser& in, const header& h) { - t.serde_async_read(in, h); + { t.serde_async_read(in, h) } -> seastar::Future; }; template concept has_serde_async_write = requires(T t, iobuf& out) { - t.serde_async_write(out); + { t.serde_async_write(out) } -> seastar::Future; }; using serde_enum_serialized_t = int32_t;