Skip to content

Conversation

clarfonthey
Copy link
Contributor

ACP: rust-lang/libs-team#235
Tracking issue: #113744

@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 16, 2023
@rust-log-analyzer

This comment has been minimized.

Comment on lines 1121 to 1135
#[stable(feature = "ip_u32", since = "1.1.0")]
impl From<Ipv4Addr> for u32 {
/// Converts an `Ipv4Addr` into a host byte order `u32`.
///
/// # Examples
///
/// ```
/// use std::net::Ipv4Addr;
///
/// let addr = Ipv4Addr::new(0x12, 0x34, 0x56, 0x78);
/// assert_eq!(0x12345678, u32::from(addr));
/// ```
#[inline]
fn from(ip: Ipv4Addr) -> u32 {
u32::from_be_bytes(ip.octets)
ip.to_bits()
}
}

#[stable(feature = "ip_u32", since = "1.1.0")]
impl From<u32> for Ipv4Addr {
/// Converts a host byte order `u32` into an `Ipv4Addr`.
///
/// # Examples
///
/// ```
/// use std::net::Ipv4Addr;
///
/// let addr = Ipv4Addr::from(0x12345678);
/// assert_eq!(Ipv4Addr::new(0x12, 0x34, 0x56, 0x78), addr);
/// ```
#[inline]
fn from(ip: u32) -> Ipv4Addr {
Ipv4Addr { octets: ip.to_be_bytes() }
Ipv4Addr::from_bits(ip)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the doc comments here since they're effectively moved to the from_bits and to_bits methods, and I assumed that the primary reason for adding doc comments to these was due to the lack of a dedicated method.

Would be fine adding them back though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nice when From impls document how they convert it, especially when it's not completely obvious. Perhaps you could link to from_bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough; does the latest change seem sufficient?

@thomcc
Copy link
Member

thomcc commented Jul 20, 2023

This has ACP+ right? The impl looks fine to me.

@thomcc
Copy link
Member

thomcc commented Jul 21, 2023

I have the impression this is fine to land by libs-api, so

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 21, 2023

📌 Commit b307014 has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2023
@bors
Copy link
Collaborator

bors commented Jul 21, 2023

⌛ Testing commit b307014 with merge 91a65364125bd1d7a11340b12874805b137289bd...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Collaborator

bors commented Jul 21, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 21, 2023
@clarfonthey
Copy link
Contributor Author

Appears to be an error with the runner itself.

@thomcc
Copy link
Member

thomcc commented Jul 21, 2023

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2023
@bors
Copy link
Collaborator

bors commented Jul 22, 2023

⌛ Testing commit b307014 with merge 8164cdb...

@bors
Copy link
Collaborator

bors commented Jul 22, 2023

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 8164cdb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 22, 2023
@bors bors merged commit 8164cdb into rust-lang:master Jul 22, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 22, 2023
@clarfonthey clarfonthey deleted the ip_bits branch July 22, 2023 16:09
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8164cdb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
7.1% [6.9%, 7.4%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-7.9% [-8.0%, -7.8%] 3
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.262s -> 651.03s (0.12%)

@thomcc
Copy link
Member

thomcc commented Jul 28, 2023

Host byte order is the system byte order, as opposed to network byte order (big endian). I would accept a docs PR clarifying the endianness conversions, though.

@clarfonthey
Copy link
Contributor Author

@thomcc I just copied the docs from the From implementations, although I agree that the wording is a bit weird. Maybe this kind of stuff is worth adding to the tracking issue if you think it's a good idea to mention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants