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.

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

Tracking issue for iter_order stabilization #27737

Closed
aturon opened this issue Aug 12, 2015 · 9 comments · Fixed by #29254
Closed

Tracking issue for iter_order stabilization #27737

aturon opened this issue Aug 12, 2015 · 9 comments · Fixed by #29254
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@aturon
Copy link
Member

aturon commented Aug 12, 2015

The iter module has an order submodule for performing comparisons on iterators. Ideally, these would instead just be implementations of e.g. the PartialOrd trait, but that trait passes by shared reference, and iterators must be mutated to be compared.

It'd be good to explore this design space, and perhaps write an RFC, before stabilization.

cc @bluss

@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@bluss
Copy link
Member

bluss commented Aug 12, 2015

One thing of note is that the origin of this API was to help libcollections, so that the collections didn't all have to reimplement correct PartialOrd eq, le, gt, etc handling*. With time, most of the collections maybe(?) will develop their own specialized versions and make it obsolete.

As noted in #26884, these methods are inferior for equality checking since they cannot assume that the two iterators are of equal length.

*Due to PartialOrd weirdness, every collection must implement all of lt, le, gt, ge.

@sfackler
Copy link
Member

It seems like these should really be methods on Iterator:

pub trait Iterator {
    ....
    fn eq<E, I>(self, other: I) -> bool where Self: Sized, Self::Item: Eq<E>, I: Iterator<Item = E> { ... }
    // etc
}

sfackler added a commit to sfackler/rust that referenced this issue Aug 27, 2015
This does cause some breakage due to deficiencies in resolve -
`path::Components` is both an `Iterator` and implements `Eq`, `Ord`,
etc. If one calls e.g. `partial_cmp` on a `Components` and passes a
`&Components` intending to target the `PartialOrd` impl, the compiler
will select the `partial_cmp` from `Iterator` and then error out. I
doubt anyone will run into breakage from `Components` specifically, but
we should see if there are third party types that will run into issues.

`iter::order::equals` wasn't moved to `Iterator` since it's exactly the
same as `iter::order::eq` but with an `Eq` instead of `PartialEq` bound,
which doensn't seem very useful.

I also updated `le`, `gt`, etc to use `partial_cmp` which lets us drop
the extra `PartialEq` bound.

cc rust-lang#27737
bors added a commit that referenced this issue Aug 27, 2015
This does cause some breakage due to deficiencies in resolve -
`path::Components` is both an `Iterator` and implements `Eq`, `Ord`,
etc. If one calls e.g. `partial_cmp` on a `Components` and passes a
`&Components` intending to target the `PartialOrd` impl, the compiler
will select the `partial_cmp` from `Iterator` and then error out. I
doubt anyone will run into breakage from `Components` specifically, but
we should see if there are third party types that will run into issues.

`iter::order::equals` wasn't moved to `Iterator` since it's exactly the
same as `iter::order::eq` but with an `Eq` instead of `PartialEq` bound,
which doensn't seem very useful.

I also updated `le`, `gt`, etc to use `partial_cmp` which lets us drop
the extra `PartialEq` bound.

cc #27737 

r? @alexcrichton
@sfackler
Copy link
Member

Would like to stabilize these in the next cycle.

@SimonSapin
Copy link
Contributor

fn eq<E, I>(self, other: I) -> bool where Self: Sized, Self::Item: Eq<E>, I: Iterator<Item = E> { ... }

By the way, is this the same as

fn eq<I>(self, other: I) -> bool where Self: Sized, Self::Item: Eq<I::Item>, I: Iterator { ... }

?

@sfackler
Copy link
Member

Your version's basically how it was actually defined when implemented: https://doc.rust-lang.org/nightly/std/iter/trait.Iterator.html#method.cmp.

@alexcrichton
Copy link
Member

This issue is now entering its cycle-long FCP for stabilization in 1.5

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Sep 24, 2015
@steveklabnik steveklabnik added this to the 1.5 milestone Oct 1, 2015
@nagisa
Copy link
Member

nagisa commented Oct 18, 2015

Can’t these be implementations of PartialEq/PartialOrd for relevant iterators instead?

@SimonSapin
Copy link
Contributor

These traits take &self, while we need at least &mut self to consume the iterators here.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the decision was to stabilize.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes rust-lang#27706
Closes rust-lang#27725
cc rust-lang#27726 (align not stabilized yet)
Closes rust-lang#27734
Closes rust-lang#27737
Closes rust-lang#27742
Closes rust-lang#27743
Closes rust-lang#27772
Closes rust-lang#27774
Closes rust-lang#27777
Closes rust-lang#27781
cc rust-lang#27788 (a few remaining methods though)
Closes rust-lang#27790
Closes rust-lang#27793
Closes rust-lang#27796
Closes rust-lang#27810
cc rust-lang#28147 (not all parts stabilized)
bors added a commit that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes #27706
Closes #27725
cc #27726 (align not stabilized yet)
Closes #27734
Closes #27737
Closes #27742
Closes #27743
Closes #27772
Closes #27774
Closes #27777
Closes #27781
cc #27788 (a few remaining methods though)
Closes #27790
Closes #27793
Closes #27796
Closes #27810
cc #28147 (not all parts stabilized)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants