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

Add std::num::Wrapping impl #1072

Merged
merged 2 commits into from
Oct 31, 2017
Merged

Add std::num::Wrapping impl #1072

merged 2 commits into from
Oct 31, 2017

Conversation

UserAB1236872
Copy link
Contributor

@UserAB1236872 UserAB1236872 commented Oct 31, 2017

Addresses #1020

This would really help with cutting down the code bloat in:

rust-random/rand#189

By making more of the structs deriveable. We'd still be blocked waiting for blanket impls that are generic over array/tuple size constants for two of the RNGs, but this would help with a lot of code.

Unfortunately, since Wrapping is in std, there's no way to do this cleanly without directly modifying serde or manually implementing for/working around every type that uses a Wrapping forever. This is an extremely small change.

@UserAB1236872 UserAB1236872 changed the title Add std::num::Wrapping support Add std::num::Wrapping impl Oct 31, 2017
Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just a question and two nits. I don't see any reason not to do this.

{
Deserialize::deserialize(deserializer).map(Wrapping)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing newline at end of file

fn deserialize<D>(deserializer: D) -> Result<Wrapping<T>, D::Error>
where D: Deserializer<'de>
{
Deserialize::deserialize(deserializer).map(Wrapping)
Copy link
Member

Choose a reason for hiding this comment

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

Deserializing a Wapping<u8> from 500 will fail. Does that match your use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify? Do you mean that if you deserialize a number that exceeds the max for that type it won't wrap? That's fine, I didn't expect it to use the wrapping functionality at deserialization time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then isn't this something Wrapping(u8::deserialize(deserializer)?) can already accomplish without this impl?

Copy link
Member

Choose a reason for hiding this comment

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

Yea that's what I meant. Just wanted to make sure. It's backwards compatible to change later if we want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hcpl If you just want to (de)serialize a Wrapping that's fine, but it becomes somewhat more problematic if you want to Derive a struct containing one. I guess you could decorate every field with a wrapping with a with attribute, but it's overall cleaner to do this.

Copy link
Contributor

@hcpl hcpl Oct 31, 2017

Choose a reason for hiding this comment

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

So there are 2 desired behaviours for Wrapping: accept and produce values in the certain range, or only do so for producing values while accepting anything.

Logically speaking, if we were to provide wrapping behaviour in impl Deserialize, we would have to do custom number parsing to wrap any possible number for consistency's sake.

Also, Wrapping<T> doesn't have any bounds on T, which means we end up having to rely on the unstable specialization feature for numeric types.

So yeah, easier to leave things this way.

{
self.0.serialize(serializer)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

also missing newline

@UserAB1236872
Copy link
Contributor Author

Should I fix the newlines in a new commit, or do an amend?

@UserAB1236872
Copy link
Contributor Author

I pushed a commit fixing the newlines, if you'd prefer it to be squashed I can do that too.

@oli-obk
Copy link
Member

oli-obk commented Oct 31, 2017

I pushed a commit fixing the newlines, if you'd prefer it to be squashed I can do that too.

fine with me

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.

Thanks!

@dtolnay dtolnay merged commit eebf0f8 into serde-rs:master Oct 31, 2017
@dtolnay
Copy link
Member

dtolnay commented Oct 31, 2017

I added some unit tests in 9b08915 and released this in Serde 1.0.17.

@dtolnay dtolnay mentioned this pull request Oct 31, 2017
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.

4 participants