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

Add Iterator comparison methods that take a comparison function #62205

Merged
merged 1 commit into from Sep 8, 2019

Conversation

@timvermeulen
Copy link
Contributor

commented Jun 28, 2019

This PR adds Iterator::{cmp_by, partial_cmp_by, eq_by, ne_by, lt_by, le_by, gt_by, ge_by}. We already have Iterator::{cmp, partial_cmp, ...} which are less general (but not any simpler) than the ones I'm proposing here.

I'm submitting this PR now because #61505 has been merged, so this change should not have a noticeable effect on the Iterator docs page size.

The diff is quite messy, here's what I changed:

  • The logic of cmp / partial_cmp / eq is moved to cmp_by / partial_cmp_by / eq_by respectively, changing x.cmp(&y) to cmp(&x, &y) in the cmp method where cmp is the given comparison function (and similar for partial_cmp_by and eq_by).
  • ne_by / lt_by / le_by / gt_by / ge_by are each implemented in terms of one of the three methods above.
  • The existing comparison methods are each forwarded to their _by counterpart, passing one of Ord::cmp / PartialOrd::partial_cmp / PartialEq::eq as the comparison function.

The corresponding _by_key methods aren't included because they're not as fundamental as the _by methods and can easily be implemented in terms of them. Is that reasonable, or would adding the _by_key methods be desirable for the sake of completeness?

I didn't add any tests – I couldn't think of any that weren't already covered by our existing tests. Let me know if there's a particular test that would be useful to add.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

Even with the docs page better, this is still a ton of new methods -- it feels like the _by_key versions aren't here because of that.

What if this only added partial_cmp_by, cmp_by, partial_cmp_by_key and cmp_by_key? How essential are the other 6/12?

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

this is still a ton of new methods -- it feels like the _by_key versions aren't here because of that.

My initial reason for not adding them was that they require the two iterators to have the same Item type, but after thinking about it a bit, that's going to be true in almost all cases anyway. So I would indeed be in favor of adding those too, the only remaining concern is the amount of new methods.

How essential are the other 6/12?

partial_cmp_by, cmp_by, and eq_by are the most fundamental ones which can derive all the others. So we could add just these three, and maybe their _by_key versions, although IMO it would still be a glaring gap in the API.

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

I thought this over some more and I think I'm coming back on ne_by / lt_by / le_by / gt_by / ge_by.

partial_cmp_by / cmp_by / eq_by stand out not just because everything else can be derived from it, but also because their return type is the same as the return type of their closure parameter. Technically that's also true for ne_by, but the given equality function determines whether two values are equal, not whether they're unequal – this may actually be harmful.

As for the others, lt_by for instance could have a closure that returns a bool indicating whether the first closure parameter is less than the second. This may actually be more useful than the current implementation, so I'm not convinced anymore that the current implementation is the best one possible. So I'm okay with only keeping partial_cmp_by / cmp_by / eq_by.

the _by_key methods are not affected by this though, so I would still rather include all of them or none at all.

@timvermeulen timvermeulen force-pushed the timvermeulen:iter_order_by branch from 2965faf to 8172a0c Jul 7, 2019

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Thanks for the PR @timvermeulen!

Do you have any real-world examples that you think are improved by these additional methods?

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@KodrAus I do for eq_by: I recently wanted to assert in a test that an iterator produced the correct floating-point values, within a certain margin of error. I couldn't create a wrapper type with a custom PartialEq::eq implementation either because the equality function wasn't transitive.

I don't have any real-world use cases for cmp_by and partial_cmp_by, but they seem fundamental enough to me to be worthwhile. I don't care too much about the other possible _by methods or the _by_key methods – unlike the methods this PR adds, they're trivial to write yourself in terms of the existing ones.

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Ping from triage, any updates? @KodrAus

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Thanks for the ping!

Ok, the eq_by, cmp_by and partial_cmp_by methods seem like reasonable additions to me.

@timvermeulen would you like to add an example for each of these methods? I know there are a bunch of methods on Iterator that don't have docs, but I think we should try keep on top of examples for new APIs as much as possible. After that I'll set up a tracking issue and this should be good to go!

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

We should also add some tests to libcore/tests/iter.rs

@edmilsonefs

This comment has been minimized.

Copy link

commented Jul 30, 2019

Hey! This is a ping from triage, we would like to know if you @timvermeulen could give us a few more minutes to update here so we can move forward.

Thanks.

@rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review

@chocol4te

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Hello! Second ping from triage, thank you for your work @timvermeulen, please feel free to reopen once you have the time (Also note that pushing to the PR while it is closed prevents it from being reopened).

@rustbot modify labels to +S-inactive-closed, -S-waiting-on-author

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

@KodrAus Sorry for the delay, how do I reopen this?

I think the closures passed to these methods should probably receive ownership of the items, since the items aren't used after being passed to the closure. I'll change that.

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

Hi @timvermeulen 👋 I'll re-open this for you

@KodrAus KodrAus reopened this Aug 25, 2019

@timvermeulen timvermeulen force-pushed the timvermeulen:iter_order_by branch 2 times, most recently from f6f6f9b to 03315ba Aug 26, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@KodrAus Thanks! I've added examples for all the involved methods, as well as some tests. I wasn't sure what to test exactly, is this more or less what you had in mind?

@timvermeulen timvermeulen force-pushed the timvermeulen:iter_order_by branch 2 times, most recently from 8856325 to b023a41 Aug 26, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

☔️ The latest upstream changes (presumably #64209) made this pull request unmergeable. Please resolve the merge conflicts.

@timvermeulen timvermeulen force-pushed the timvermeulen:iter_order_by branch 3 times, most recently from 0e95881 to 37a683e Sep 6, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-06T12:47:44.8896981Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-06T12:47:44.9082508Z ##[command]git config gc.auto 0
2019-09-06T12:47:44.9167032Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-06T12:47:44.9228962Z ##[command]git config --get-all http.proxy
2019-09-06T12:47:44.9374128Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/62205/merge:refs/remotes/pull/62205/merge
---
2019-09-06T12:54:38.6407606Z    Compiling serde_json v1.0.40
2019-09-06T12:54:40.5923386Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-09-06T12:54:52.1793364Z     Finished release [optimized] target(s) in 1m 35s
2019-09-06T12:54:52.1876304Z tidy check
2019-09-06T12:54:52.6987477Z tidy error: /checkout/src/libcore/iter/traits/iterator.rs: ignoring file length unnecessarily
2019-09-06T12:54:54.2959219Z some tidy checks failed
2019-09-06T12:54:54.2964256Z 
2019-09-06T12:54:54.2964256Z 
2019-09-06T12:54:54.2965966Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-09-06T12:54:54.2971572Z 
2019-09-06T12:54:54.2971871Z 
2019-09-06T12:54:54.2982870Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-09-06T12:54:54.2983388Z Build completed unsuccessfully in 0:01:39
2019-09-06T12:54:54.2983388Z Build completed unsuccessfully in 0:01:39
2019-09-06T12:54:54.3083686Z == clock drift check ==
2019-09-06T12:54:54.3139945Z   local time: Fri Sep  6 12:54:54 UTC 2019
2019-09-06T12:54:54.4646759Z   network time: Fri, 06 Sep 2019 12:54:54 GMT
2019-09-06T12:54:54.4647152Z == end clock drift check ==
2019-09-06T12:54:55.7773425Z ##[error]Bash exited with code '1'.
2019-09-06T12:54:55.7807176Z ##[section]Starting: Checkout
2019-09-06T12:54:55.7808947Z ==============================================================================
2019-09-06T12:54:55.7809025Z Task         : Get sources
2019-09-06T12:54:55.7809073Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@timvermeulen timvermeulen force-pushed the timvermeulen:iter_order_by branch from 37a683e to 58ba1f5 Sep 6, 2019

@KodrAus
KodrAus approved these changes Sep 8, 2019
Copy link
Contributor

left a comment

Ok this looks good to me!

Thanks @timvermeulen

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

📌 Commit 58ba1f5 has been approved by KodrAus

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

⌛️ Testing commit 58ba1f5 with merge bfb6714...

bors added a commit that referenced this pull request Sep 8, 2019
Auto merge of #62205 - timvermeulen:iter_order_by, r=KodrAus
Add Iterator comparison methods that take a comparison function

This PR adds `Iterator::{cmp_by, partial_cmp_by, eq_by, ne_by, lt_by, le_by, gt_by, ge_by}`. We already have `Iterator::{cmp, partial_cmp, ...}` which are less general (but not any simpler) than the ones I'm proposing here.

I'm submitting this PR now because #61505 has been merged, so this change should not have a noticeable effect on the `Iterator` docs page size.

The diff is quite messy, here's what I changed:
- The logic of `cmp` / `partial_cmp` / `eq` is moved to `cmp_by` / `partial_cmp_by` / `eq_by` respectively, changing `x.cmp(&y)` to `cmp(&x, &y)` in the `cmp` method where `cmp` is the given comparison function (and similar for `partial_cmp_by` and `eq_by`).
- `ne_by` / `lt_by` / `le_by` / `gt_by` / `ge_by` are each implemented in terms of one of the three methods above.
- The existing comparison methods are each forwarded to their `_by` counterpart, passing one of `Ord::cmp` / `PartialOrd::partial_cmp` / `PartialEq::eq` as the comparison function.

The corresponding `_by_key` methods aren't included because they're not as fundamental as the `_by` methods and can easily be implemented in terms of them. Is that reasonable, or would adding the `_by_key` methods be desirable for the sake of completeness?

I didn't add any tests – I couldn't think of any that weren't already covered by our existing tests. Let me know if there's a particular test that would be useful to add.
Centril added a commit to Centril/rust that referenced this pull request Sep 8, 2019
Rollup merge of rust-lang#62205 - timvermeulen:iter_order_by, r=KodrAus
Add Iterator comparison methods that take a comparison function

This PR adds `Iterator::{cmp_by, partial_cmp_by, eq_by, ne_by, lt_by, le_by, gt_by, ge_by}`. We already have `Iterator::{cmp, partial_cmp, ...}` which are less general (but not any simpler) than the ones I'm proposing here.

I'm submitting this PR now because rust-lang#61505 has been merged, so this change should not have a noticeable effect on the `Iterator` docs page size.

The diff is quite messy, here's what I changed:
- The logic of `cmp` / `partial_cmp` / `eq` is moved to `cmp_by` / `partial_cmp_by` / `eq_by` respectively, changing `x.cmp(&y)` to `cmp(&x, &y)` in the `cmp` method where `cmp` is the given comparison function (and similar for `partial_cmp_by` and `eq_by`).
- `ne_by` / `lt_by` / `le_by` / `gt_by` / `ge_by` are each implemented in terms of one of the three methods above.
- The existing comparison methods are each forwarded to their `_by` counterpart, passing one of `Ord::cmp` / `PartialOrd::partial_cmp` / `PartialEq::eq` as the comparison function.

The corresponding `_by_key` methods aren't included because they're not as fundamental as the `_by` methods and can easily be implemented in terms of them. Is that reasonable, or would adding the `_by_key` methods be desirable for the sake of completeness?

I didn't add any tests – I couldn't think of any that weren't already covered by our existing tests. Let me know if there's a particular test that would be useful to add.
@Centril Centril referenced this pull request Sep 8, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Sep 8, 2019
Auto merge of #64281 - Centril:rollup-inyqjf8, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #62205 (Add Iterator comparison methods that take a comparison function)
 - #64152 (Use backtrace formatting from the backtrace crate)
 - #64265 (resolve: Mark more erroneous imports as used)
 - #64267 (rustdoc: fix diagnostic with mixed code block styles)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

⌛️ Testing commit 58ba1f5 with merge e8216f8...

bors added a commit that referenced this pull request Sep 8, 2019
Auto merge of #62205 - timvermeulen:iter_order_by, r=KodrAus
Add Iterator comparison methods that take a comparison function

This PR adds `Iterator::{cmp_by, partial_cmp_by, eq_by, ne_by, lt_by, le_by, gt_by, ge_by}`. We already have `Iterator::{cmp, partial_cmp, ...}` which are less general (but not any simpler) than the ones I'm proposing here.

I'm submitting this PR now because #61505 has been merged, so this change should not have a noticeable effect on the `Iterator` docs page size.

The diff is quite messy, here's what I changed:
- The logic of `cmp` / `partial_cmp` / `eq` is moved to `cmp_by` / `partial_cmp_by` / `eq_by` respectively, changing `x.cmp(&y)` to `cmp(&x, &y)` in the `cmp` method where `cmp` is the given comparison function (and similar for `partial_cmp_by` and `eq_by`).
- `ne_by` / `lt_by` / `le_by` / `gt_by` / `ge_by` are each implemented in terms of one of the three methods above.
- The existing comparison methods are each forwarded to their `_by` counterpart, passing one of `Ord::cmp` / `PartialOrd::partial_cmp` / `PartialEq::eq` as the comparison function.

The corresponding `_by_key` methods aren't included because they're not as fundamental as the `_by` methods and can easily be implemented in terms of them. Is that reasonable, or would adding the `_by_key` methods be desirable for the sake of completeness?

I didn't add any tests – I couldn't think of any that weren't already covered by our existing tests. Let me know if there's a particular test that would be useful to add.
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Sep 8, 2019
Auto merge of #64281 - Centril:rollup-inyqjf8, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #62205 (Add Iterator comparison methods that take a comparison function)
 - #64152 (Use backtrace formatting from the backtrace crate)
 - #64265 (resolve: Mark more erroneous imports as used)
 - #64267 (rustdoc: fix diagnostic with mixed code block styles)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

⌛️ Testing commit 58ba1f5 with merge 2c0931e...

@bors bors merged commit 58ba1f5 into rust-lang:master Sep 8, 2019

4 of 5 checks passed

homu Testing commit 58ba1f51efab249411a932880e3c10d36355b166 with merge 2c0931e168671d7536b58563dc3664c948a8dcd3...
Details
pr Build #20190906.29 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@KodrAus KodrAus referenced this pull request Sep 8, 2019
0 of 2 tasks complete
@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

Alrighty, I've opened a tracking issue in #64295 for this feature and will submit a follow-up PR to update our issue number to reference it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.