Skip to content

Clarify network byte order conversions for integer / IP address conversions. #49418

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

Merged
merged 1 commit into from
Apr 1, 2018

Conversation

frewsxcv
Copy link
Member

Opened primarily to address #48819.

Also added a few other conversion docs/examples.

@rust-highfive
Copy link
Contributor

r? @TimNN

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2018
@@ -769,7 +769,16 @@ impl FromInner<c::in_addr> for Ipv4Addr {

#[stable(feature = "ip_u32", since = "1.1.0")]
impl From<Ipv4Addr> for u32 {
/// It performs the conversion in network order (big-endian).
/// Convert an `Ipv4Addr` into a host byte order `u32`.
Copy link
Member Author

Choose a reason for hiding this comment

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

admittedly i'm not sure what this terminology should be here w.r.t. the removal of 'network order' and addition of 'host byte order'. i just took it as a suggestion from this thread. might be nice to get input from the libs/docs team here @rust-lang/libs @rust-lang/docs

Copy link
Contributor

Choose a reason for hiding this comment

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

To me the new wording seems good, and more easily understandable than the old version, although I'd be curious to hear what other people think.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. The new wording looks good, and this is how I'd expect this function to behave.

@@ -778,21 +787,48 @@ impl From<Ipv4Addr> for u32 {

#[stable(feature = "ip_u32", since = "1.1.0")]
impl From<u32> for Ipv4Addr {
/// It performs the conversion in network order (big-endian).
/// Convert an host byte order `u32` into an `Ipv4Addr`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this should be "a host byte [...]" (At least it should be consistent with the doc comment above).

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! addressed in the latest force push

@frewsxcv frewsxcv force-pushed the frewsxcv-network-order branch from e95ccc7 to b89fb71 Compare March 28, 2018 11:17
@clarfonthey
Copy link
Contributor

Why doesn't this include the u128 impls as well?

@frewsxcv
Copy link
Member Author

Why doesn't this include the u128 impls as well?

ran out of time when i made this branch. we can expand to the the u128 impls in a future pr

@TimNN
Copy link
Contributor

TimNN commented Mar 30, 2018

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 30, 2018

📌 Commit b89fb71 has been approved by TimNN

@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 Mar 30, 2018
@bors
Copy link
Collaborator

bors commented Apr 1, 2018

⌛ Testing commit b89fb71 with merge cb1f898...

bors added a commit that referenced this pull request Apr 1, 2018
Clarify network byte order conversions for integer / IP address conversions.

Opened primarily to address #48819.

Also added a few other conversion docs/examples.
@bors
Copy link
Collaborator

bors commented Apr 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: TimNN
Pushing cb1f898 to master...

@bors bors merged commit b89fb71 into rust-lang:master Apr 1, 2018
@frewsxcv frewsxcv deleted the frewsxcv-network-order branch April 1, 2018 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants