serde: fix c++23 enum handling #24853
Conversation
Signed-off-by: Noah Watkins <[email protected]>
The is_serde check enabled in c++23 (prior to this commit) checked only for scoped enums. So the SFINAE magic for enum wasn't being triggered for non-scoped enum. Instead, it was probably being handled by scalar/integer encoding. Indeed, a not-enum guard is present on scalar customization points, but, not-enum in c++23, again, means not scoped enum. Long ago we informally banned non-scoped enums. Turns out, there was a non-scoped enum being used in serde. See previous commit for that. This commit changes serde so that it retains the previous behavior that worked in c++20, namely, the use of std::is_enum which checks the superset of scoped and non-scoped enums. Signed-off-by: Noah Watkins <[email protected]>
|
@IoannisRP we still haven't looked into the 64-bit-enum-falls-back-to-scalar thing. but that'd be a good one to check out too. |
| @@ -24,6 +24,7 @@ namespace serde { | |||
| template<typename T> | |||
| requires(serde_is_enum_v<std::decay_t<T>>) | |||
| void tag_invoke(tag_t<write_tag>, iobuf& out, T t) { | |||
| static_assert(std::is_scoped_enum_v<std::decay_t<T>>); | |||
There was a problem hiding this comment.
don't let your friends use non-scoped enums
There was a problem hiding this comment.
template< class T >struct is_scoped_enum; (since C++23) will not work with CMake :-(
There was a problem hiding this comment.
it works with Bazel, and when I tried to switch cmake to c++23 it broke the upgrade tests because this was an incompatible change to the serialization format for this enum
There was a problem hiding this comment.
oh I see, we need to guard the static assert with c++ version checks
There was a problem hiding this comment.
Maybe write in a more verbose way that would work with both c++20 and c++23 until we fully switch? Something like this.
There was a problem hiding this comment.
i don't think we need to do anything here, right? we can run CI on the vtools side with c++23 turned on?
There was a problem hiding this comment.
Yeah but that's going to require the annoying commit vtools PR and this PR "together" song and dance. I'm fine with doing that (glad that we don't have to do this anymore with Bazel).
There was a problem hiding this comment.
LOL i independently used the phrasing "song and dance" in the PR i just pushed which made the same observation
There was a problem hiding this comment.
Maybe write in a more verbose way that would work with both c++20 and c++23 until we fully switch? Something like this.
Good idea. I think we can compile it out of the tree for now, though, because in principle the fix only needs to last a day or two until vtools switches over to c++23.
| inline std::ostream& | ||
| operator<<(std::ostream& os, const incremental_update_operation& op) { | ||
| return os << incremental_update_operation_as_string(op); | ||
| } |
There was a problem hiding this comment.
nit: Once we have this operator, can we omit incremental_update_operation_as_string function call in operator<<(std::ostream& o, const property_update<T>& p) and operator<<(std::ostream& o, const property_update<tristate<T>>& p) to get rid of this function?
This is so that we can run CI without having to do the song and dance on vtools side to select the proper dev branch. Signed-off-by: Noah Watkins <[email protected]>
See commit messages. Changes
cluster::incremental_update_operationto use scoped enum.Backports Required
Release Notes