Implement PartialOrd and Ord for SocketAddr* #72239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (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. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Aren't trait impls insta-stable? |
|
To be honest, I wasn't really sure.. which is why I played it safe by making them unstable. Should I just change all the instances of Edit: Done. |
|
Uh, I'll rebase it properly in a moment. |
|
r? @Amanieu |
|
I'm not really sure if ordering addresses makes sense, but since we already have the precedent in @rfcbot fcp merge |
|
@rfcbot fcp merge |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
This FCP covers: // heterogeneous PartialEq
impl PartialEq<SocketAddr> for SocketAddrV4 {...}
impl PartialEq<SocketAddr> for SocketAddrV6 {...}
impl PartialEq<SocketAddrV4> for SocketAddr {...}
impl PartialEq<SocketAddrV6> for SocketAddr {...}
// Ord
impl Ord for SocketAddr {...}
impl Ord for SocketAddrV4 {...}
impl Ord for SocketAddrV6 {...}
// homogeneous PartialOrd
impl PartialOrd<SocketAddr> for SocketAddr {...}
impl PartialOrd<SocketAddrV4> for SocketAddrV4 {...}
impl PartialOrd<SocketAddrV6> for SocketAddrV6 {...}
// heterogeneous PartialOrd
impl PartialOrd<SocketAddr> for SocketAddrV4 {...}
impl PartialOrd<SocketAddr> for SocketAddrV6 {...}
impl PartialOrd<SocketAddrV4> for SocketAddr {...}
impl PartialOrd<SocketAddrV6> for SocketAddr {...}The first 3 groups looks fine to me. Is there any use case for the last group? What kind of code would reasonably want to use those? |
|
Just gonna register my skepticism in #72239 (comment). @rfcbot concern heterogeneous PartialOrd I think those impls could cause more harm than good, where a refactor turns something that used to be a homogeneous |
|
I implemented it because Though I agree that its use cases are very niche and generally not useful, it may be surprising for users to find that their code fails to compile, when they find-and-replace |
|
Removed heterogeneous ordering as per @dtolnay's suggestion and updated the tests a little bit. The PR should be ready for further (and hopefully final!) review now. |
nytopop
added a commit
to nytopop/blip
that referenced
this pull request
May 24, 2020
This makes it possible to search a MultiNodeCut by SocketAddr without doing a linear scan. Unfortunately, SocketAddr doesn't implement Ord, so we have to base the sort on tuples of (ip, port). related: rust-lang/rust#72239
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
@bors r+ |
|
📌 Commit d1bc8ad has been approved by |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 29, 2020
Rollup of 12 pull requests Successful merges: - rust-lang#72239 (Implement PartialOrd and Ord for SocketAddr*) - rust-lang#72466 (Stabilize str_strip feature) - rust-lang#72605 (Add working example for E0617 explanation) - rust-lang#72636 (Cleanup `Resolver::<clone|into>_outputs` methods) - rust-lang#72645 (Add myself to .mailmap) - rust-lang#72667 (expand unaligned_references test) - rust-lang#72670 (Fix incorrect comment in generator test) - rust-lang#72674 (Clippy should always build) - rust-lang#72682 (Add test for rust-lang#66930) - rust-lang#72695 (update data layout for illumos x86) - rust-lang#72697 (Remove rustc-ux-guidelines) - rust-lang#72702 (rustc_lint: Remove `unused_crate_dependencies` from the `unused` group) Failed merges: r? @ghost
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Jun 13, 2020
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Jun 14, 2020
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
zonyitoo
referenced
this pull request
in shadowsocks/shadowsocks-rust
Jun 15, 2020
ACL checking may requires lots of calculation and network I/O (DNS resolution)
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Jun 15, 2020
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Jun 15, 2020
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Aug 6, 2020
While here clean up all pkglint warnings. Changes since 1.44.1: Version 1.45.2 (2020-08-03) ========================== * [Fix bindings in tuple struct patterns][74954] * [Fix track_caller integration with trait objects][74784] [74954]: rust-lang/rust#74954 [74784]: rust-lang/rust#74784 Version 1.45.1 (2020-07-30) ========================== * [Fix const propagation with references.][73613] * [rustfmt accepts rustfmt_skip in cfg_attr again.][73078] * [Avoid spurious implicit region bound.][74509] * [Install clippy on x.py install][74457] [73613]: rust-lang/rust#73613 [73078]: rust-lang/rust#73078 [74509]: rust-lang/rust#74509 [74457]: rust-lang/rust#74457 Version 1.45.0 (2020-07-16) ========================== Language -------- - [Out of range float to int conversions using `as` has been defined as a saturating conversion.][71269] This was previously undefined behaviour, but you can use the `{f64, f32}::to_int_unchecked` methods to continue using the current behaviour, which may be desirable in rare performance sensitive situations. - [`mem::Discriminant<T>` now uses `T`'s discriminant type instead of always using `u64`.][70705] - [Function like procedural macros can now be used in expression, pattern, and statement positions.][68717] This means you can now use a function-like procedural macro anywhere you can use a declarative (`macro_rules!`) macro. Compiler -------- - [You can now override individual target features through the `target-feature` flag.][72094] E.g. `-C target-feature=+avx2 -C target-feature=+fma` is now equivalent to `-C target-feature=+avx2,+fma`. - [Added the `force-unwind-tables` flag.][69984] This option allows rustc to always generate unwind tables regardless of panic strategy. - [Added the `embed-bitcode` flag.][71716] This codegen flag allows rustc to include LLVM bitcode into generated `rlib`s (this is on by default). - [Added the `tiny` value to the `code-model` codegen flag.][72397] - [Added tier 3 support\* for the `mipsel-sony-psp` target.][72062] - [Added tier 3 support for the `thumbv7a-uwp-windows-msvc` target.][72133] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`net::{SocketAddr, SocketAddrV4, SocketAddrV6}` now implements `PartialOrd` and `Ord`.][72239] - [`proc_macro::TokenStream` now implements `Default`.][72234] - [You can now use `char` with `ops::{Range, RangeFrom, RangeFull, RangeInclusive, RangeTo}` to iterate over a range of codepoints.][72413] E.g. you can now write the following; ```rust for ch in 'a'..='z' { print!("{}", ch); } println!(); // Prints "abcdefghijklmnopqrstuvwxyz" ``` - [`OsString` now implements `FromStr`.][71662] - [The `saturating_neg` method as been added to all signed integer primitive types, and the `saturating_abs` method has been added for all integer primitive types.][71886] - [`Arc<T>`, `Rc<T>` now implement `From<Cow<'_, T>>`, and `Box` now implements `From<Cow>` when `T` is `[T: Copy]`, `str`, `CStr`, `OsStr`, or `Path`.][71447] - [`Box<[T]>` now implements `From<[T; N]>`.][71095] - [`BitOr` and `BitOrAssign` are implemented for all `NonZero` integer types.][69813] - [The `fetch_min`, and `fetch_max` methods have been added to all atomic integer types.][72324] - [The `fetch_update` method has been added to all atomic integer types.][71843] Stabilized APIs --------------- - [`Arc::as_ptr`] - [`BTreeMap::remove_entry`] - [`Rc::as_ptr`] - [`rc::Weak::as_ptr`] - [`rc::Weak::from_raw`] - [`rc::Weak::into_raw`] - [`str::strip_prefix`] - [`str::strip_suffix`] - [`sync::Weak::as_ptr`] - [`sync::Weak::from_raw`] - [`sync::Weak::into_raw`] - [`char::UNICODE_VERSION`] - [`Span::resolved_at`] - [`Span::located_at`] - [`Span::mixed_site`] - [`unix::process::CommandExt::arg0`] Cargo ----- Misc ---- - [Rustdoc now supports strikethrough text in Markdown.][71928] E.g. `~~outdated information~~` becomes "~~outdated information~~". - [Added an emoji to Rustdoc's deprecated API message.][72014] Compatibility Notes ------------------- - [Trying to self initialize a static value (that is creating a value using itself) is unsound and now causes a compile error.][71140] - [`{f32, f64}::powi` now returns a slightly different value on Windows.][73420] This is due to changes in LLVM's intrinsics which `{f32, f64}::powi` uses. - [Rustdoc's CLI's extra error exit codes have been removed.][71900] These were previously undocumented and not intended for public use. Rustdoc still provides a non-zero exit code on errors. Internals Only -------------- - [Make clippy a git subtree instead of a git submodule][70655] - [Unify the undo log of all snapshot types][69464] [73420]: rust-lang/rust#73420 [72324]: rust-lang/rust#72324 [71843]: rust-lang/rust#71843 [71886]: rust-lang/rust#71886 [72234]: rust-lang/rust#72234 [72239]: rust-lang/rust#72239 [72397]: rust-lang/rust#72397 [72413]: rust-lang/rust#72413 [72014]: rust-lang/rust#72014 [72062]: rust-lang/rust#72062 [72094]: rust-lang/rust#72094 [72133]: rust-lang/rust#72133 [71900]: rust-lang/rust#71900 [71928]: rust-lang/rust#71928 [71662]: rust-lang/rust#71662 [71716]: rust-lang/rust#71716 [71447]: rust-lang/rust#71447 [71269]: rust-lang/rust#71269 [71095]: rust-lang/rust#71095 [71140]: rust-lang/rust#71140 [70655]: rust-lang/rust#70655 [70705]: rust-lang/rust#70705 [69984]: rust-lang/rust#69984 [69813]: rust-lang/rust#69813 [69464]: rust-lang/rust#69464 [68717]: rust-lang/rust#68717 [`Arc::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.as_ptr [`BTreeMap::remove_entry`]: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.remove_entry [`Rc::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr [`rc::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.as_ptr [`rc::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.from_raw [`rc::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.into_raw [`sync::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.as_ptr [`sync::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.from_raw [`sync::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.into_raw [`str::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_prefix [`str::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_suffix [`char::UNICODE_VERSION`]: https://doc.rust-lang.org/stable/std/char/constant.UNICODE_VERSION.html [`Span::resolved_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.resolved_at [`Span::located_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.located_at [`Span::mixed_site`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.mixed_site [`unix::process::CommandExt::arg0`]: https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.arg0
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 24, 2023
…shtriplett Derive `Ord`, `PartialOrd` and `Hash` for `SocketAddr*` Fixes rust-lang#116711 The main pain of this PR is to fix the buggy impl of `Ord` for `SocketAddrV6`, which ignored half of the fields (while `PartialEq` is derived): https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L99-L106 https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L676 For me it looks like a simple copy-paste error made in rust-lang#72239 (copy from v4 impl) (cc `@hch12907),` as I don't see this behavior being mentioned anywhere on the PR and it also does not respect `cmp` trait "rules". I also do not see any reasons for those impls to _not_ be derived. It's a shame we did not notice this for 28 versions/3 years. I guess this is a bug fix, but I'm not sure what the process here should be. r? libs
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 24, 2023
Rollup merge of rust-lang#116714 - WaffleLapkin:order-the-order, r=joshtriplett Derive `Ord`, `PartialOrd` and `Hash` for `SocketAddr*` Fixes rust-lang#116711 The main pain of this PR is to fix the buggy impl of `Ord` for `SocketAddrV6`, which ignored half of the fields (while `PartialEq` is derived): https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L99-L106 https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L676 For me it looks like a simple copy-paste error made in rust-lang#72239 (copy from v4 impl) (cc `@hch12907),` as I don't see this behavior being mentioned anywhere on the PR and it also does not respect `cmp` trait "rules". I also do not see any reasons for those impls to _not_ be derived. It's a shame we did not notice this for 28 versions/3 years. I guess this is a bug fix, but I'm not sure what the process here should be. r? libs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation is mostly the same as the one found in
IpAddr(other than adding comparison for ports, of course).Continues #53788 and #53863
Fixes #53710