Change RangeInclusive to a three-field struct. #51622

kennytm · 2018-06-18T21:05:35Z

This PR also reverts #48012 (i.e. removed the try_fold/try_rfold specialization for RangeInclusive) because LLVM no longer has trouble recognizing a RangeInclusive loop.

rust-highfive · 2018-06-18T21:05:38Z

r? @cramertj

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

cramertj · 2018-06-18T21:51:24Z

r? @SimonSapin

cuviper · 2018-06-18T23:19:19Z

src/libcore/ops/range.rs

+#[stable(feature = "inclusive_range", since = "1.26.0")]
+impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> {
+    fn eq(&self, other: &Self) -> bool {
+        self.start == other.start && self.end == other.end


Are you sure you don't need to compare is_iterating? Consider:

let a = 0..=0; let mut b = a.clone(); assert_eq!(a, b); assert_eq!(b.next(), Some(0)); assert_eq!(b.next(), None); assert_eq!(a, b); // yes or no?

Currently that last assert would fail, because an exhausted range is replaced by 1..=0. I think with your new code, they will look equal, only different by their is_iterating.

Another one to consider:

let a = 1..=10; let mut b = 0..=10; b.next(); assert_eq!(a, b); // same `start` and `end`, different `is_iterating`

I guess is_empty() is the better value to use for PartialEq and Hash, but that needs PartialOrd.

On this other hand, do we expect this to always be true?

fn reflexive(r: RangeInclusive) -> bool { r == (r.start()..=r.end()) }

Imo, it would be quite jarring if it were not always true..

Agreed with @SimonSapin and @Centril that taking is_iterating into account when comparing is strange. At the very least we need to modify the impl Debug to expose is_iterating if we do want to make the field significant.

On this other hand, do we expect this to always be true?

fn reflexive(r: RangeInclusive) -> bool {
r == (r.start()..=r.end())
}

That works in the status quo because the range is changed to 1..=0 when the iterator is exhausted. But it troubles me that with this change, r and r.start()..=r.end() might compare equal yet not produce the same items, if r is exhausted.

Right. Both "compares equal when reconstructed form accessors" and "yields the same items if compares equal" seem desirable, but we can’t have both with this PR’s three-fields design.

SimonSapin · 2018-06-19T05:09:35Z

I think the replace_one and replace_zero methods of the Step trait can also be removed, right?

I can review for correctness in a bit, but I really don’t have an opinion on whether this change is desirable in the first place.

kennytm · 2018-06-19T09:06:52Z

I think the replace_one and replace_zero methods of the Step trait can also be removed, right?

Seems like yes, I can't find other uses of these two methods in this repo.

These two functions were added by #34530 as substitute of std::num::{Zero, One}, well before we've chosen the 1..=0 representation. Though they were also only used in RangeInclusive.

kennytm · 2018-06-19T09:20:15Z

cc #51601, these two PRs are going to silently conflict with each other.

kennytm · 2018-06-19T18:40:18Z

Turns out LLVM 3.9 cannot recognize the loop. LLVM 6.0 is fine. Not sure about LLVM 4.0.

Emerentius · 2018-06-20T00:27:01Z

@kennytm so, uh, first to get his PR merged wins and loser has to clean up the conflict?

kennytm · 2018-06-20T06:30:02Z

@Emerentius yeah 😐

Emerentius · 2018-06-21T11:15:03Z

@kennytm I guess I win then 😛

TimNN · 2018-06-26T17:15:30Z

Ping from triage, @SimonSapin: This PR requires your review.

SimonSapin · 2018-06-26T17:29:15Z

I believe that #51622 (comment) is an unresolved issue.

When the index is not PartialOrd, always treat the range as empty.

kennytm · 2018-07-13T05:09:51Z

@bors r=SimonSapin

bors · 2018-07-13T05:09:52Z

📌 Commit dbfa8fe2ca143818b94ded7b49e6511cfd4ff330 has been approved by SimonSapin

…tter.

bors · 2018-07-13T05:26:37Z

📌 Commit 6093128 has been approved by SimonSapin

bors · 2018-07-13T05:50:35Z

⌛ Testing commit 6093128 with merge 00ebf67226780da280a20f4ec60d3c62adb9b58f...

bors · 2018-07-13T06:56:08Z

💔 Test failed - status-travis

rust-highfive · 2018-07-13T06:56:10Z

The job dist-armv7-linux of your PR failed on Travis (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.

  0     0    0     0    0     0      0      0 --:--:--  0:00:54 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:55 --:--:--     0curl: (6) Could not resolve host: s3-us-west-1.amazonaws.com
[01:03:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host armv7-unknown-linux-gnueabihf --target armv7-unknown-linux-gnueabihf
[01:03:16] Build completed unsuccessfully in 1:00:09
[01:03:16] thread 'main' panicked at 'failed to download openssl source: exit code: 6', bootstrap/native.rs:589:17
travis_time:end:023c19d0:start=1531461117067340622,finish=1531464913566915344,duration=3796499574722

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
travis_time:start:1c7fc56b
---
travis_time:end:0b258f91:start=1531464914135306042,finish=1531464914142374925,duration=7068883
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00406488
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1573a0de
$ dmesg | grep -i kill

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)

rust-highfive · 2018-07-13T06:56:15Z

The job dist-armv7-linux of your PR failed on Travis (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.

  0     0    0     0    0     0      0      0 --:--:--  0:00:54 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:55 --:--:--     0curl: (6) Could not resolve host: s3-us-west-1.amazonaws.com
[01:03:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host armv7-unknown-linux-gnueabihf --target armv7-unknown-linux-gnueabihf
[01:03:16] Build completed unsuccessfully in 1:00:09
[01:03:16] thread 'main' panicked at 'failed to download openssl source: exit code: 6', bootstrap/native.rs:589:17
travis_time:end:023c19d0:start=1531461117067340622,finish=1531464913566915344,duration=3796499574722

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
travis_time:start:1c7fc56b
---
travis_time:end:0b258f91:start=1531464914135306042,finish=1531464914142374925,duration=7068883
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00406488
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1573a0de
$ dmesg | grep -i kill

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)

kennytm · 2018-07-13T10:02:59Z

@bors retry travis-ci/travis-ci#9696

bors · 2018-07-13T10:18:45Z

⌛ Testing commit 6093128 with merge c0955a3...

Change RangeInclusive to a three-field struct. Fix #45222. This PR also reverts #48012 (i.e. removed the `try_fold`/`try_rfold` specialization for `RangeInclusive`) because LLVM no longer has trouble recognizing a RangeInclusive loop.

bors · 2018-07-13T12:19:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing c0955a3 to master...

rust-highfive assigned cramertj Jun 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2018

rust-highfive assigned SimonSapin and unassigned cramertj Jun 18, 2018

This comment has been minimized.

Sign in to view

cuviper reviewed Jun 18, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

kennytm force-pushed the three-field-range-inclusive branch from 08e3aad to b0e29d9 Compare June 19, 2018 10:27

This comment has been minimized.

Sign in to view

kennytm force-pushed the three-field-range-inclusive branch from b0e29d9 to 5c80508 Compare June 19, 2018 15:14

This comment has been minimized.

Sign in to view

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2018

kennytm force-pushed the three-field-range-inclusive branch from 5c80508 to 4941579 Compare June 19, 2018 18:39

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2018

kennytm force-pushed the three-field-range-inclusive branch from 4941579 to ccb0166 Compare June 22, 2018 17:42

This comment has been minimized.

Sign in to view

kennytm force-pushed the three-field-range-inclusive branch from ccb0166 to bb0d05a Compare June 22, 2018 19:24

kennytm added 3 commits July 13, 2018 09:53

Fix some RangeInclusive test cases.

d299f03

Upgrade implementation of StepBy<RangeInclusive<_>>.

b6ea93e

Include is_empty() in PartialEq and Hash.

6e0dd9e

When the index is not PartialOrd, always treat the range as empty.

kennytm force-pushed the three-field-range-inclusive branch from 5e3bd09 to dbfa8fe Compare July 13, 2018 05:08

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2018

This comment has been minimized.

Sign in to view

Changed implementation of the third field to make LLVM optimize it be…

6093128

…tter.

kennytm force-pushed the three-field-range-inclusive branch from dbfa8fe to 6093128 Compare July 13, 2018 05:26

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2018

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 Jul 13, 2018

bors merged commit 6093128 into rust-lang:master Jul 13, 2018

kennytm deleted the three-field-range-inclusive branch July 13, 2018 12:37

c410-f3r mentioned this pull request Aug 5, 2018

Implement Serialize and Deserialize for RangeInclusive serde-rs/serde#1347

Merged

scottmcm mentioned this pull request Dec 15, 2018

Override <RangeInclusive as Iterator>::try_(r)fold #56563

Closed

Mark-Simulacrum mentioned this pull request Jan 31, 2020

PartialEq implementation for RangeInclusive is unsound #67194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change RangeInclusive to a three-field struct. #51622

Change RangeInclusive to a three-field struct. #51622

kennytm commented Jun 18, 2018

rust-highfive commented Jun 18, 2018

cramertj commented Jun 18, 2018

This comment has been minimized.

cuviper Jun 18, 2018

SimonSapin Jun 19, 2018

Centril Jun 19, 2018

kennytm Jun 19, 2018

cuviper Jun 19, 2018

SimonSapin Jun 19, 2018

SimonSapin commented Jun 19, 2018

kennytm commented Jun 19, 2018

kennytm commented Jun 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kennytm commented Jun 19, 2018 •

edited

Loading

Emerentius commented Jun 20, 2018

kennytm commented Jun 20, 2018

Emerentius commented Jun 21, 2018

This comment has been minimized.

TimNN commented Jun 26, 2018

SimonSapin commented Jun 26, 2018

kennytm commented Jul 13, 2018 •

edited

Loading

bors commented Jul 13, 2018

This comment has been minimized.

bors commented Jul 13, 2018

bors commented Jul 13, 2018

bors commented Jul 13, 2018

rust-highfive commented Jul 13, 2018

rust-highfive commented Jul 13, 2018

kennytm commented Jul 13, 2018

bors commented Jul 13, 2018

bors commented Jul 13, 2018

Change RangeInclusive to a three-field struct. #51622

Change RangeInclusive to a three-field struct. #51622

Conversation

kennytm commented Jun 18, 2018

rust-highfive commented Jun 18, 2018

cramertj commented Jun 18, 2018

This comment has been minimized.

cuviper Jun 18, 2018

Choose a reason for hiding this comment

SimonSapin Jun 19, 2018

Choose a reason for hiding this comment

Centril Jun 19, 2018

Choose a reason for hiding this comment

kennytm Jun 19, 2018

Choose a reason for hiding this comment

cuviper Jun 19, 2018

Choose a reason for hiding this comment

SimonSapin Jun 19, 2018

Choose a reason for hiding this comment

SimonSapin commented Jun 19, 2018

kennytm commented Jun 19, 2018

kennytm commented Jun 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kennytm commented Jun 19, 2018 • edited Loading

Emerentius commented Jun 20, 2018

kennytm commented Jun 20, 2018

Emerentius commented Jun 21, 2018

This comment has been minimized.

TimNN commented Jun 26, 2018

SimonSapin commented Jun 26, 2018

kennytm commented Jul 13, 2018 • edited Loading

bors commented Jul 13, 2018

This comment has been minimized.

bors commented Jul 13, 2018

bors commented Jul 13, 2018

bors commented Jul 13, 2018

rust-highfive commented Jul 13, 2018

rust-highfive commented Jul 13, 2018

kennytm commented Jul 13, 2018

bors commented Jul 13, 2018

bors commented Jul 13, 2018

kennytm commented Jun 19, 2018 •

edited

Loading

kennytm commented Jul 13, 2018 •

edited

Loading