Use to_ne_bytes for converting IPv4Addr to octets
#57740
Conversation
It is easier and it should be also faster, because `to_ne_bytes` just calls `mem::transmute`.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TimNN (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
JakubOnderka
changed the title
to_ne_bytes for converting IPv4Address to octetsto_ne_bytes for converting IPv4Addr to octets
| @@ -393,8 +393,7 @@ impl Ipv4Addr { | |||
| /// ``` | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub fn octets(&self) -> [u8; 4] { | |||
| let bits = u32::from_be(self.inner.s_addr); | |||
| [(bits >> 24) as u8, (bits >> 16) as u8, (bits >> 8) as u8, bits as u8] | |||
| self.inner.s_addr.to_ne_bytes() | |||
There was a problem hiding this comment.
It's surprising to me that there was an explicit big-endian mention here before, but now it's using native-endian. Obviously the doctest is passing on the little-endian test machine, but are we sure this is correct? Is it possible that, for clarity, it should be written differently? Maybe
let bits = u32::from_be(self.inner.s_addr);
bits.to_be_bytes()(It's still fully efficient, as LLVM knows that bswap(bswap(x)) == x.)
There was a problem hiding this comment.
Yes, because s_addr is always in big endian order and octets methods returns data in big endian order too. So we just wanna return bytes as they are and yes, in that case is to_ne_bytes little lie. But I am not sure if your proposed solution will more clarify that, maybe write some comment?
There was a problem hiding this comment.
If it's right that's the important thing :)
(A comment with your first sentence there might be nice, though. Up to you.)
frewsxcv
added
the
T-libs-api
|
r? @scottmcm |
|
@bors r+ rollup |
|
📌 Commit 87f5a98 has been approved by |
bors
added
S-waiting-on-bors
…r=scottmcm Use `to_ne_bytes` for converting IPv4Addr to octets It is easier and it should be also faster, because [`to_ne_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.to_ne_bytes) just calls `mem::transmute`.
|
Could we run a perf for this PR? |
…r=scottmcm Use `to_ne_bytes` for converting IPv4Addr to octets It is easier and it should be also faster, because [`to_ne_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.to_ne_bytes) just calls `mem::transmute`.
…r=scottmcm Use `to_ne_bytes` for converting IPv4Addr to octets It is easier and it should be also faster, because [`to_ne_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.to_ne_bytes) just calls `mem::transmute`.
Rollup of 16 pull requests Successful merges: - #57259 (Update reference of rlibc crate to compiler-builtins crate) - #57740 (Use `to_ne_bytes` for converting IPv4Addr to octets) - #57926 (Tiny expansion to docs for `core::convert`.) - #58157 (Add Cargo.lock automatically adding message) - #58203 (rustdoc: display sugared return types for async functions) - #58243 (Add trait alias support in rustdoc) - #58262 (Add #[must_use] message to Fn* traits) - #58295 (std::sys::unix::stdio: explain why we do into_raw) - #58297 (Cleanup JS a bit) - #58317 (Some writing improvement, conciseness of intro) - #58324 (miri: give non-generic functions a stable address) - #58332 (operand-to-place copies should never be overlapping) - #58345 (When there are multiple filenames, print what got interpreted as filenames) - #58346 (rpath computation: explain why we pop()) - #58350 (Fix failing tidy (line endings on Windows)) - #58352 (miri value visitor: use `?` in macro) Failed merges: r? @ghost
|
@lzutao There's no need; the ASM (at least on x86) is identical. And the perf benchmarks wouldn't find a difference regardless, since none of them use this enough to show up. |
Now that we have {to|from}_be_bytes the code can be simpler.
(Inspired by PR rust-lang#57740)
…=oli-obk
Use less explicit shifting in std::net::ip
Now that we have `{to|from}_be_bytes` the code can be simpler.
(Inspired by PR rust-lang#57740)
…=oli-obk
Use less explicit shifting in std::net::ip
Now that we have `{to|from}_be_bytes` the code can be simpler.
(Inspired by PR rust-lang#57740)
…=oli-obk
Use less explicit shifting in std::net::ip
Now that we have `{to|from}_be_bytes` the code can be simpler.
(Inspired by PR rust-lang#57740)
It is easier and it should be also faster, because
to_ne_bytesjust callsmem::transmute.