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

Remove Ipv4Addr::is_ietf_protocol_assignment #86439

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Remove Ipv4Addr::is_ietf_protocol_assignment #86439

merged 1 commit into from
Aug 2, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jun 18, 2021 •
edited
Loading

This PR removes the unstable method Ipv4Addr::is_ietf_protocol_assignment, as I suggested in #85612 (comment). The method was added in #60145, as far as I can tell primarily for the implementation of Ipv4Addr::is_global (addresses reserved for IETF protocol assignment are not globally reachable unless otherwise specified).

The method was added in 2019, but I haven't been able to find any open-source code using this method so far. I'm also having a hard time coming up with a usecase for specifically this method; knowing that an address is reserved for future protocols doesn't allow you to do much with it, especially since now some of those addresses are indeed assigned to a protocol and have their own behaviour (and might even be defined to be globally reachable, so if that is what you care about it is always more accurate to call !is_global(), instead of is_ietf_protocol_assignment()).

Because of these reasons, I propose removing the method (or alternatively make it a private helper for is_global) and also not introduce Ipv6Addr::is_ietf_protocol_assignment and IpAddr::is_ietf_protocol_assignment in the future.

@rustbot rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 18, 2021
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Jun 18, 2021
@CDirkx CDirkx force-pushed the ip-protocol-assignment branch from 850648c to a26237e Compare June 19, 2021 13:04
CDirkx
CDirkx commented Jun 19, 2021
View reviewed changes
library/std/src/net/ip/tests.rs
Comment on lines -452 to +446
check!("192.0.0.0", ietf_protocol_assignment);
check!("192.0.0.255", ietf_protocol_assignment);
check!("192.0.0.100", ietf_protocol_assignment);
check!("192.0.0.0");
check!("192.0.0.255");
check!("192.0.0.100");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I had removed these lines entirely, but I decided to keep them as a check that these addresses are not global; is_global should not return true for these addresses.

@CDirkx

This comment has been minimized.

Sign in to view
@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2021
@CDirkx CDirkx mentioned this pull request Jun 25, 2021
Open
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Aug 1, 2021

Thanks.

If anyone ever does find a use for this function, we can always add it back again.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2021

📌 Commit a26237e has been approved by m-ou-se

@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 1, 2021
@JohnTitor JohnTitor mentioned this pull request Aug 2, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2021
@bors
Auto merge of rust-lang#87689 - JohnTitor:rollup-ns38b56, r=JohnTitor
effea9a 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#86183 (Change environment variable getters to error recoverably)
 - rust-lang#86439 (Remove `Ipv4Addr::is_ietf_protocol_assignment`)
 - rust-lang#86509 (Move `os_str_bytes` to `sys::unix`)
 - rust-lang#86593 (Partially stabilize `const_slice_first_last`)
 - rust-lang#86936 (Add documentation for `Ipv6MulticastScope`)
 - rust-lang#87282 (Ensure `./x.py dist` adheres to `build.tools`)
 - rust-lang#87468 (Update rustfmt)
 - rust-lang#87504 (Update mdbook.)
 - rust-lang#87608 (Remove unused field `Session.system_library_path`)
 - rust-lang#87629 (Consistent spelling of "adapter" in the standard library)
 - rust-lang#87633 (Update compiler_builtins to fix i128 shift/mul on thumbv6m)
 - rust-lang#87644 (Recommend `swap_remove` in `Vec::remove` docs)
 - rust-lang#87653 (mark a UB doctest as no_run)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a03d6da into rust-lang:master Aug 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 2, 2021
@dtolnay dtolnay mentioned this pull request Sep 28, 2023
Open
28 tasks
jstasiak added a commit to jstasiak/rust-libs-team that referenced this pull request Dec 7, 2023
@jstasiak
Remove no longer existing IP address methods from ipcheck
47f89e0 
The is_unicast_site_local() method has been gone since Rust 1.55[1] and
is_ietf_protocol_assignment() since 1.56[2].

I had to run this tool to test something, saw it wouldn't build with
modern Rust.

[1] rust-lang/rust#85820
[2] rust-lang/rust#86439
@jstasiak jstasiak mentioned this pull request Dec 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
7 participants
@CDirkx @rust-highfive @m-ou-se @bors @JohnCSimon @camelid @rustbot