Add faster serde serialization, sacrificing invariant-safety for speed in release mode; test that it roudtrips #252
Conversation
|
r? @erickt on the custom serialization I didn't use the |
|
cc @dtolnay Don't do this, it'll break formats like json that need to wrap aggregates in We got a ticket open to write a container-level attribute that'll do this for you. |
| HostInternal::None => 0u8.serialize(serializer), | ||
| HostInternal::Domain => 1u8.serialize(serializer), | ||
| HostInternal::Ipv4(addr) => { | ||
| 2u8.serialize(serializer)?; |
There was a problem hiding this comment.
would be better as (2u8, addr).serialize(serializer)
| addr.serialize(serializer) | ||
| } | ||
| HostInternal::Ipv6(addr) => { | ||
| 3u8.serialize(serializer)?; |
|
Actually, |
…d in release mode; test that it roudtrips
|
Picked up the code generator from #254, and added a travis check to ensure that its in sync. r? @SimonSapin |
|
FYI, once we implement serde-rs/serde#642, you should be able to replace your custom serde impls for |
|
@Manishearth I'm very much against that patch. No invalid URL should be produced. Why was that needed? |
…g regular Serialize impl
|
Split it out as an extra pair of methods |
|
Why must we put into the repos generated code? |
|
Need it to work on stable and don't want heavy dependencies on aster/syntex. It's temporary till macros 1.1 comes out. Trying to keep the dependency list small for now. |
Doesn’t serde with macros 1.1 use aster and syntax? |
|
No, it uses syn, and doesn't need build scripts. |
|
serde can use syn with a build script on stable without needing syntex. I would rather we do that instead of checking in generated code. |
| #[cfg(feature="serde")] | ||
| impl Url { | ||
| /// Serialize the URL efficiently, for use with IPC | ||
| pub fn serialize_efficient<S>(&self, serializer: &mut S) -> Result<(), S::Error> where S: serde::Serializer { |
There was a problem hiding this comment.
Please don't name if efficient. Its main property is that it is risky, not efficient.
| query_start: query_start, | ||
| fragment_start: fragment_start | ||
| }; | ||
| #[cfg(debug_assertions)] { |
There was a problem hiding this comment.
Without debug_assertions, type errors in that block won't be emitted. Do if cfg!(debug_assertions) instead.
Fair. I also want to avoid a build script. And I want to avoid new dependencies for as long as possible, until https://bugzilla.mozilla.org/show_bug.cgi?id=1322798 is resolved. Right now the state of rust-url dependencies is acceptable for Gecko, but until that issue is resolved I'd prefer to wait before adding extra dependencies that can be avoided. |
I would rather we make things more complicated on the Gecko side than on the Rust side for every contributor stumbling upon generated code. |
|
The only generated code is for the host struct, which isn't modified very often. |
|
And if we use syn+build script on this side you still have to deal with generated code. And debugging gets harder to do. |
|
Closing in favor of #259, which is largely based on this. |
r? @SimonSapin
This change is