serde/test: serde_fields for various structs #23123
Conversation
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/53730#01919bd3-4668-4736-a62f-d25e449273fa ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/53973#0191ba71-17c6-444b-907d-583aa09b1076 |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/53730#01919beb-e7d1-4580-978d-6fded9606fcb: |
| @@ -657,6 +678,7 @@ struct new_cs | |||
| bool operator==(const new_cs&) const = default; | |||
|
|
|||
| std::vector<test_msg1_new_manual> data_; | |||
| auto serde_fields() { return std::tie(data_); } | |||
There was a problem hiding this comment.
so this is the case where writes go to serde_fields, but reads go through read_nested?
There was a problem hiding this comment.
yeah, there may be others in this file. i've seen it in actual live code as well, but an example hasn't sprung to mind.
There was a problem hiding this comment.
cool. i think then a follow up to the final PR we might want to consider either serde_fields or writing both read/write methods explicitly. can discuss separately this tho.
There was a problem hiding this comment.
you mean like xor, right? as in disallow cases like this one?
There was a problem hiding this comment.
yeh, xor. i guess we talked about that before. nm
There was a problem hiding this comment.
all good. i agree - will need another round of updates & reviews, so no harm in punting to follow up.
| @@ -693,6 +715,7 @@ struct old_cs | |||
| bool operator==(const old_cs&) const = default; | |||
|
|
|||
| std::vector<test_msg1_new_manual> data_; | |||
| auto serde_fields() { return std::tie(data_); } | |||
There was a problem hiding this comment.
the template name old_cs and old_no_cs in CRTP look wrong (unless its on purpose for the test)
There was a problem hiding this comment.
sigh. these are all over the tests. no clue how to guess whether it's intentional.
There was a problem hiding this comment.
we can ask @felixguendling and see if he has a memory
There was a problem hiding this comment.
I think this is a copy & paste mistake I made. Back then, the type was not used by the envelope class at all, so maybe this is why it was overlooked.
There was a problem hiding this comment.
Cool, thanks @felixguendling. I've made this mistake a handful of times, and I only catch it by visual inspection. Not sure whether it's testable in a general way in practice. Maybe as a clang-tidy check or something.
There was a problem hiding this comment.
I tried something, but due to the cyclic nature, it didn't really work:
#include <type_traits>
template <typename T>
struct p {
using value_t = T;
// does not compile
// definition of 'p<c>' is not complete until the closing '}'
// static_assert(std::is_assignable_v<p<T>, T>);
};
struct c : p<c> {};
static_assert(std::is_assignable_v<p<c>, c>); // correct, no problem
struct w : p<c> {};
static_assert(std::is_assignable_v<p<w>, w>); // assertion failshttps://gcc.godbolt.org/z/baTocTqG9
So, yes - probably only a clang-tidy check or something similar would help.
There was a problem hiding this comment.
With some manual overhead, you can check every envelope type after it's defined:
#include <type_traits>
template <typename T>
struct p {
using value_t = T;
};
template <typename T>
concept Good = std::is_assignable_v<T, typename T::value_t>;
struct c : p<c> {};
static_assert(Good<c>);
struct w : p<c> {};
static_assert(Good<w>); // failsThere was a problem hiding this comment.
cyclic nature
Yeah, exactly. Manual check lgtm; I could envision adding a debug build step where we scrape headers for envelopes then generate and collect static asserts in a separate compilation unit, but that's probably rather expensive to compile. Besides which it wouldn't cover this case, where the envelope is isolated in a .cc file.
| @@ -703,6 +726,8 @@ struct new_no_cs | |||
|
|
|||
| serde::checksum_t unchecked_dummy_checksum_{0U}; | |||
| std::vector<test_msg1_new_manual> data_; | |||
|
|
|||
| auto serde_fields() { return std::tie(unchecked_dummy_checksum_, data_); } | |||
There was a problem hiding this comment.
this one has them flipped the other way, so maybe its intentional
There was a problem hiding this comment.
oh i see. yeah, those value types have something to do with operator overloading, but the need (or lack of need, as the case may be) for those operators is mysterious to me.
|
/ci-repeat 1 |
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
oleiman
force-pushed
the
serde-fields/serde-test
branch
from
87c38cf
to
ba5516b
Compare
|
force push to correct a few CRTP typos in |
In #22782, we aim to require explicit serde_fields for serde structs that would otherwise use aggregate field order. This is one in a series of pull requests that add serde_fields to existing structs, to be followed by a PR to remove the aggregate order fallback from serde itself.
The primary acceptance criterion for this PR should be "field order unchanged". Any functional change is a bug.
Backports Required
Release Notes