search

TimelyDataflow / abomonation

A mortifying serialization library for Rust
MIT License
317 stars 30 forks source link

Implement Abomonation for the rest of the primitive data types #14

Closed sdht0 closed 5 years ago

sdht0 commented 5 years ago

Adding missing implementations for the 128 bit and NonZero* types.

I'm not 100% sure about the NonZero* types implementation, but since they are just a newtype wrapper over the primitive types, this should simply work?

sdht0 commented 5 years ago

Hmm, if this is accepted as is, the minimum rust version requirement will bump up to 1.34.0.

frankmcsherry commented 5 years ago

Minimum Rust version bump seems fine (it's already pretty close, due to 2018 edition stuffs)!

I'm up for merging, but I'll have to apologize to @antiguru who has had a u128 PR languishing. The main anxiety here is that I am less certain that unaligned 128 bit reads are defined behavior on as many platforms as are unaligned 64 bit reads. Philosophically, abomonation may be on some slow creep towards "not ever well defined behavior", at which point we can take it to v2.0.

The NonZero* stuff seems sane, and I think the surrounding code usually does the right thing (e.g. an Option<NonZero*> should be correct as a copy, whereas Option<T> needed a bit more care when T was not a primitive type).

sdht0 commented 5 years ago

Oops, I should have checked the existing PRs/issues! #11 and #12 can probably be closed now though.

Are there plans for a new release of abomonation? I had to change the abomonation path to my local instance all over the place in Cargo.toml of Timely and Differential to make use of the NonZero* implementation.

Thanks!

frankmcsherry commented 5 years ago

Is it possible that NonZeroI* are not stable yet?

sdht0 commented 5 years ago

They were stabilized in 1.34: https://github.com/rust-lang/rust/blob/master/RELEASES.md#stabilized-apis-1

frankmcsherry commented 5 years ago

Weirdly, when I try and build it locally I'm seeing a bunch of 

error[E0412]: cannot find type `NonZeroI8` in this scope
   --> src/lib.rs:291:22
    |
291 | impl Abomonation for NonZeroI8 { }
    |                      ^^^^^^^^^ help: a struct with a similar name exists: `NonZeroU8`

I'm sure its something dumb, but I'll need to sort this out before pushing a new release!

frankmcsherry commented 5 years ago

I just upgraded to 1.35 and nothing seemed to change here. Hrm...

sdht0 commented 5 years ago

Wild guess: did you import std::num::*?

frankmcsherry commented 5 years ago

It is at the top of the file, yes. Is that a .. bad thing? (I think style wise it is, but I don't see why it would block access to these folks).

frankmcsherry commented 5 years ago

Mystery solved: rustup had a nightly override, and nightly hasn't been able to update through rustup for several versions now. Removing the override fixes it.

frankmcsherry commented 5 years ago

https://crates.io/crates/abomonation

sdht0 commented 5 years ago

Ah. I was going to suggest trying nightly.

This was quick, thanks a lot!