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

No definition of lexicographic order in docs for Iterator::cmp* #72255

Closed
brmmm3 opened this issue May 16, 2020 · 6 comments · Fixed by #78347
Closed

No definition of lexicographic order in docs for Iterator::cmp* #72255

brmmm3 opened this issue May 16, 2020 · 6 comments · Fixed by #78347
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brmmm3
Copy link

brmmm3 commented May 16, 2020

It seems that the comparison functions are not correctly implemented.

I tried this code:

use std::cmp::Ordering;

assert_eq!([1].iter().cmp([1, 2].iter()), Ordering::Less);
assert_eq!([1, 2].iter().cmp([1].iter()), Ordering::Greater);

I expected to see this happen:
The code above should panic. The reason is: 1 is NOT less than 1 and 1 is NOT greater than 1.
What I'm missing in Ordering are LessOrEqual and GreaterOrEqual. If these 2 objects would exist I could write:

use std::cmp::Ordering;

assert_eq!([1].iter().cmp([1, 2].iter()), Ordering::LessOrEqual);
assert_eq!([1, 2].iter().cmp([1].iter()), Ordering::GreaterOrEqual);

which would correct not panic.

Meta

I've tried this in Rust Playground with stable and nightly.

@brmmm3 brmmm3 added the C-bug Category: This is a bug. label May 16, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 16, 2020

Quoting the docs for the C++ standard library (emphasis mine):

Lexicographical comparison is a operation with the following properties:

  • Two ranges are compared element by element.
  • The first mismatching element defines which range is lexicographically less or greater than the other.
  • If one range is a prefix of another, the shorter range is lexicographically less than the other.
  • If two ranges have equivalent elements and are of the same length, then the ranges are lexicographically equal.
  • An empty range is lexicographically less than any non-empty range.
  • Two empty ranges are lexicographically equal.

Iterator::cmp performs a lexicographical comparison. We should probably define lexicographical order wherever the term is used in the standard library documentation.

@ecstatic-morse ecstatic-morse added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed C-bug Category: This is a bug. labels May 16, 2020
@ecstatic-morse ecstatic-morse changed the title Wrong implementation of cmp* in std::iter::Iterator No definition of lexicographic order in docs for Iterator::cmp* May 16, 2020
@ecstatic-morse ecstatic-morse added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label May 21, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 21, 2020

To resolve this, you can either write a definition of lexicographical order somewhere in the rust documentation or link to an easily understandable source. Then link to that definition wherever "lexicographical" appears in the source. FWIW, I don't think the wikipedia article qualifies as "easily understandable".

@softwaredeveloptam
Copy link

Hi I would like to take this on as a first issue if that's okay.
@rustbot claim

@rustbot rustbot self-assigned this Jun 5, 2020
@crlf0710 crlf0710 added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jun 10, 2020
@Alexendoo
Copy link
Member

Triage: Hi, are you still working on this issue @softwaredeveloptam?

@softwaredeveloptam
Copy link

Hey there, @Alexendoo I got pretty busy and it slipped my mind, but will give it a shot this week!

@Rustin170506
Copy link
Member

I am working on this.
@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants