SoftbearStudios / bitcode

A binary encoder/decoder for Rust
https://crates.io/crates/bitcode/
MIT License
334 stars 17 forks source link

Add support for `std::net::{*Addr*}` #30

Closed finnbear closed 6 days ago

finnbear commented 1 month ago

Works by converting them to/from existing implementations like Result<Ipv4Addr, Ipv6Addr>, u32, u128, and (Ipv4Addr, u16). This is to avoid additional unsafe code. Manual re-implementation might yield better performance.

inodentry commented 1 month ago

What about SocketAddr? It's basically ip address + port, I imagine it shouldn't be hard to support. It's very common in network protocol messages.

finnbear commented 1 month ago

What about SocketAddr?

Hmm; SocketAddr is tricker because we would have to decide how to handle SocketAddrV6's flowinfo and scope_id. impl serde::Serialize for SocketAddrV6 omits them and impl serde::Deserialize for SocketAddrV6 zero-initializes them. This leads to better JSON, for example, but could lead to data loss. Perhaps bitcode, which doesn't have a text format, should keep all the fields?

Edit:

poll: :tada: ip + port + flowinfo + scope_id :confused: ip + port + flowinfo :rocket: ip + port

inodentry commented 1 month ago

I agree with your logic that a binary non-human-readable format does not need to worry about being "pretty" and can use that to its advantage to be more complete/correct.

Further, encoding the extra fields is strictly more useful than not encoding them.

If I want to just store an IP address + port, I can just make them two separate fields in my struct (IpAddr + u16) and then construct a SocketAddr from that. It is therefore possible to "opt out" of encoding them, if the size of the encoded message is more important than preserving those fields, or if I don't care about them.

However, if the encoding omits them, now I have no way of preserving them, if I happen to have a use case where they are actually important. There is no way to "opt in".

Therefore, my opinion is that they should be encoded.

finnbear commented 1 month ago

Thank you, I personally agree! (this PR is now awaiting @caibear's review, although I already use it via a git dependency)