Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Ser+De for Saturating<T> #2709

Merged
merged 1 commit into from Apr 11, 2024
Merged

Conversation

jbethune
Copy link

@jbethune jbethune commented Mar 4, 2024

This implementation is heavily inspired by the existing trait implentation for std::num::Wrapping<T>.

fix #2708

@jbethune
Copy link
Author

jbethune commented Mar 4, 2024

std::num::Saturating<T> was added in Rust version 1.74 which is above the minimum supported version of Serde. How should we handle this?

@jbethune
Copy link
Author

I've added the rustversion crate to version-gate the implementation so that it also compiles for older Rust versions.

@oli-obk
Copy link
Member

oli-obk commented Mar 27, 2024

We do version detection in build.rs:

if minor < 64 {
Please use that scheme to register a new cfg and put your new additions behind such a cfg

@oli-obk
Copy link
Member

oli-obk commented Mar 27, 2024

(also once done, please rebase and squash your commits)

@jbethune
Copy link
Author

(also once done, please rebase and squash your commits)

Done. Thanks for the clarifications!

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good.

One question I think we should consider below.

I think it would help to understand more about your use case where these impls are going to be needed.

test_suite/tests/test_de_error.rs Outdated Show resolved Hide resolved
@jbethune
Copy link
Author

your use case where these impls are going to be needed.

I'm making a game for a virtual 16 bit architecture with number semantics that saturate at 2^16-1 because that's the maximum level you can achieve for different stats.

@jbethune jbethune requested a review from oli-obk April 4, 2024 20:18
@jbethune
Copy link
Author

jbethune commented Apr 4, 2024

@oli-obk I have updated the deserialization code so that it maps exceedingly large or small values to the MAX and MIN values of the target type.

I will squash and rebase (if needed) after I have an updated approval from you.

Thanks!

serde/src/de/impls.rs Outdated Show resolved Hide resolved
serde/src/de/impls.rs Outdated Show resolved Hide resolved
@jbethune
Copy link
Author

jbethune commented Apr 5, 2024

@oli-obk I have incorporated your requests and suggestions and squashed everything into one commit

The serialization implementation is heavily
inspired by the existing trait implentation for
`std::num::Wrapping<T>`.

The deserializing implementation maps input values
that lie outside of the numerical range of the
output type to the `MIN` or `MAX` value of the
output type, depending on the sign of the input
value. This behaviour follows to the `Saturating`
semantics of the output type.

fix serde-rs#2708
@oli-obk oli-obk merged commit a6571ee into serde-rs:master Apr 11, 2024
15 checks passed
@oli-obk
Copy link
Member

oli-obk commented Apr 11, 2024

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Apr 16, 2024

Thanks — published in serde 1.0.198.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Impl Serialize/Deserialize for std::num::Saturating<T>
3 participants