Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Minor changes to Ipv4Addr #75110

Merged
merged 1 commit into from
Aug 24, 2020
Merged

Minor changes to Ipv4Addr #75110

merged 1 commit into from
Aug 24, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 3, 2020 •
edited
Loading

Minor changes to Ipv4Addr

  • Impl IntoInner rather than AsInner for Ipv4Addr
  • Add some comments
  • Add test to show endiannes of Ipv4Addr display

tesuji
tesuji commented Aug 3, 2020
View reviewed changes
library/std/src/net/addr.rs Outdated Show resolved Hide resolved
@tesuji tesuji force-pushed the ip-endianness branch 3 times, most recently from 083b739 to 2c5e292 Compare August 3, 2020 16:27
@tesuji tesuji mentioned this pull request Aug 3, 2020
Merged
cuviper
cuviper reviewed Aug 3, 2020
View reviewed changes
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
cuviper
cuviper reviewed Aug 3, 2020
View reviewed changes
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@tesuji tesuji mentioned this pull request Aug 4, 2020
Closed
@tesuji tesuji force-pushed the ip-endianness branch 3 times, most recently from 13ada0d to 833dbb5 Compare August 5, 2020 03:15
@tesuji tesuji marked this pull request as ready for review August 5, 2020 03:17
tesuji
tesuji commented Aug 5, 2020
View reviewed changes
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
tesuji
tesuji commented Aug 5, 2020
View reviewed changes
library/std/src/net/addr.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

pickfire commented Aug 5, 2020

Do we have like benchmark for this to see if it improves performance?

@tesuji
Copy link
Contributor Author

tesuji commented Aug 5, 2020

Do we have like benchmark for this to see if it improves performance?

Never claimed this PR will improve performance. Rather I try to enforce endianness of Ipv4Addr,
but I am not sure effectiveness of it in the future.

RalfJung
RalfJung reviewed Aug 5, 2020
View reviewed changes
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Aug 6, 2020
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

I left some more comments; with that, this is fine from a UCG perspective.

But I can't judge if this is actually a change that we want or if it improves the readability of the code here -- I find it about equally difficult to follow before and after. Somehow highfive did not assign a reviewer, so r? @Mark-Simulacrum

library/std/src/net/addr.rs Outdated Show resolved Hide resolved
library/std/src/net/addr.rs Outdated Show resolved Hide resolved
library/std/src/net/endian.rs Outdated Show resolved Hide resolved
library/std/src/net/endian.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2020

And again for the bot to see: r? @Mark-Simulacrum

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2020

(I have no idea who would be a good reviewer and thanks to the library/ rename I can't easily access the history of this file to find out. Feel free to reassign to someone more appropriate.)

@tesuji
Copy link
Contributor Author

tesuji commented Aug 6, 2020

(You could use git blame locally).

@tesuji
Copy link
Contributor Author

tesuji commented Aug 6, 2020

Thanks a lot about your help here, @RalfJung . I am really happy about that.

@Mark-Simulacrum
Copy link
Member

Please update the description in the top-level comment to match what this PR does after the latest changes, and squash the commits into one. With that done I think this will be good to go.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 17, 2020

Done.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit 3b2c0a9 has been approved by Mark-Simulacrum

@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 Aug 18, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
@tmandry
Rollup merge of rust-lang#75110 - lzutao:ip-endianness, r=Mark-Simula…
81f62a9 
…crum

Minor changes to Ipv4Addr

Minor changes to Ipv4Addr

* Impl IntoInner rather than AsInner for Ipv4Addr
* Add some comments
* Add test to show endiannes of Ipv4Addr display
@tmandry tmandry mentioned this pull request Aug 18, 2020
Closed
@tmandry
Copy link
Member

tmandry commented Aug 18, 2020 •
edited
Loading

Failed in #75683 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 18, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 19, 2020 •
edited
Loading

Thanks. The issue happened because in netbsd, in_addr is repr(packed).
I haven't found repr(packed) struct in_addr in other platforms in libc crate.

@tesuji
Minor changes to Ipv4Addr
768509f 
* Impl IntoInner rather than AsInner for Ipv4Addr
* Add some comments
* Add test to show endiannes of Ipv4Addr display
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2020

📌 Commit 768509f has been approved by Mark-Simulacrum

@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 Aug 24, 2020
@bors
Copy link
Contributor

bors commented Aug 24, 2020

⌛ Testing commit 768509f with merge f44c6e4...

@bors
Copy link
Contributor

bors commented Aug 24, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing f44c6e4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2020
@bors bors merged commit f44c6e4 into rust-lang:master Aug 24, 2020
@bors bors mentioned this pull request Aug 24, 2020
Closed
@tesuji tesuji deleted the ip-endianness branch August 24, 2020 23:45
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

@RalfJung RalfJung RalfJung left review comments

@oli-obk oli-obk oli-obk left review comments

@pickfire pickfire pickfire left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

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.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
9 participants
@tesuji @pickfire @RalfJung @oli-obk @bors @Mark-Simulacrum @tmandry @cuviper @rustbot