-
Notifications
You must be signed in to change notification settings - Fork 143
Bump serde to 1.0, bincode to 0.8 and bump version #164
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
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon. |
Why are we reverting to an earlier version of bincode? |
Unless I misunderstood something, bincode did 1.0-alpha => 0.8 Might be me though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the silly question: does the Deserialize
bounds change actually break the public interface of ipc-channel
, or only make it more powerful?...
src/ipc.rs
Outdated
let mut data = &*self.data; | ||
let mut deserializer = bincode::Deserializer::new(&mut data, bincode::Infinite); | ||
let data = bincode::read_types::SliceReader::new(&self.data[..]); | ||
let mut deserializer = bincode::Deserializer::new(data, bincode::Infinite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the binding better be called something else than data
after this change?... (reader
perhaps? Or maybe data_reader
?...)
@Eijebong note that while it's true that I don't know how to decide which one is more appropriate for us to use, though. |
pinging @nox who is the upgrade master for advice |
Pinging @gankro too who is the last who has touched bincode stuff. What's the current state of art over there? Why is there |
@Eijebong: bincode 0.8.0 is indeed the most recent release |
|
||
#[derive(Debug)] | ||
pub struct IpcReceiver<T> where T: Deserialize + Serialize { | ||
pub struct IpcReceiver<T> where T: for<'de> Deserialize<'de> + Serialize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd check with @dtolnay, but I'm pretty sure that since you can't taking advantage of zero-copy parsing, you could replace all of these lifetime bounds with Deserialize<'static>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this really make a difference ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserialize<'static>
is never what you want. for<'de> Deserialize<'de>
or DeserializeOwned
is correct. See https://serde.rs/lifetimes.html#trait-bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/ipc.rs
Outdated
let mut data = &*self.data; | ||
let mut deserializer = bincode::Deserializer::new(&mut data, bincode::Infinite); | ||
let reader = bincode::read_types::SliceReader::new(&self.data[..]); | ||
let mut deserializer = bincode::Deserializer::new(reader, bincode::Infinite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason that you aren't just using bincode::deserialize
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because I didn't know it existed. I'll try with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it seems to be working.
@bors-servo: r+ |
📌 Commit 7d0c49b has been approved by |
Bump serde to 1.0, bincode to 0.8 and bump version
☀️ Test successful - status-appveyor, status-travis |
No description provided.