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

override `VecDeque`'s `Iter::try_fold` #57974

Merged
merged 1 commit into from Jan 30, 2019

Conversation

Projects
None yet
6 participants
@llogiq
Copy link
Contributor

llogiq commented Jan 29, 2019

This should improve performance (wherever it is used), but I haven't found the time to benchmark it yet.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 29, 2019

r? @alexcrichton

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 29, 2019

Thanks! Can you be sure to add some tests to exercise this overriddent implementation in a few possible edge cases?

@llogiq llogiq force-pushed the llogiq:vec-deque-try-fold branch from c86e77b to 7e22cb2 Jan 29, 2019

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 29, 2019

Sure thing.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 29, 2019

@bors: r+

Thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 29, 2019

📌 Commit 7e22cb2 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Jan 30, 2019

Rollup merge of rust-lang#57974 - llogiq:vec-deque-try-fold, r=alexcr…
…ichton

override `VecDeque`'s `Iter::try_fold`

This should improve performance (wherever it is used), but I haven't found the time to benchmark it yet.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 30, 2019

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:0f450735:start=1548803812323085264,finish=1548803885287012601,duration=72963927337
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:14:33] 
[01:14:33] running 119 tests
[01:14:58] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:15:03] i......iii.i.....ii
[01:15:03] 
[01:15:03]  finished in 29.242
[01:15:03] travis_fold:end:test_debuginfo

---
[01:22:06]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[01:22:15] error[E0382]: use of moved value: `v`
[01:22:15]     --> src/liballoc/../liballoc/tests/vec_deque.rs:1466:21
[01:22:15]      |
[01:22:15] 1466 |             let r = v[n];
[01:22:15]      |                     ^ value used here after move
[01:22:15] ...
[01:22:15] 1471 |         assert_eq!(Ok::<_, ()>(66), v.into_iter().try_fold(0, |a, b| Ok(a + b)));
[01:22:15]      |                                     - value moved here
[01:22:15]      |
[01:22:15]      = note: move occurs because `v` has type `std::collections::VecDeque<usize>`, which does not implement the `Copy` trait
[01:22:15] error[E0382]: use of moved value: `v`
[01:22:15]     --> src/liballoc/../liballoc/tests/vec_deque.rs:1467:13
[01:22:15]      |
[01:22:15]      |
[01:22:15] 1467 |             v.rotate_left(n ^ r);
[01:22:15]      |             ^ value used here after move
[01:22:15] ...
[01:22:15] 1471 |         assert_eq!(Ok::<_, ()>(66), v.into_iter().try_fold(0, |a, b| Ok(a + b)));
[01:22:15]      |                                     - value moved here
[01:22:15]      |
[01:22:15]      = note: move occurs because `v` has type `std::collections::VecDeque<usize>`, which does not implement the `Copy` trait
[01:22:15] error[E0382]: use of moved value: `v`
[01:22:15]     --> src/liballoc/../liballoc/tests/vec_deque.rs:1469:13
[01:22:15]      |
[01:22:15]      |
[01:22:15] 1469 |             v.rotate_right(n % 11);
[01:22:15]      |             ^ value used here after move
[01:22:15] 1470 |         }
[01:22:15] 1471 |         assert_eq!(Ok::<_, ()>(66), v.into_iter().try_fold(0, |a, b| Ok(a + b)));
[01:22:15]      |                                     - value moved here
[01:22:15]      |
[01:22:15]      = note: move occurs because `v` has type `std::collections::VecDeque<usize>`, which does not implement the `Copy` trait
[01:22:15] error[E0382]: use of moved value: `v`
[01:22:15]     --> src/liballoc/../liballoc/tests/vec_deque.rs:1471:37
[01:22:15]      |
[01:22:15]      |
[01:22:15] 1471 |         assert_eq!(Ok::<_, ()>(66), v.into_iter().try_fold(0, |a, b| Ok(a + b)));
[01:22:15]      |                                     ^ value moved here in previous iteration of loop
[01:22:15]      |
[01:22:15]      = note: move occurs because `v` has type `std::collections::VecDeque<usize>`, which does not implement the `Copy` trait
[01:22:15] error: aborting due to 4 previous errors
[01:22:15] 
[01:22:15] For more information about this error, try `rustc --explain E0382`.
[01:22:15] error: Could not compile `alloc`.
[01:22:15] error: Could not compile `alloc`.
[01:22:15] warning: build failed, waiting for other jobs to finish...
[01:22:20] error: build failed
[01:22:20] 
[01:22:20] 
[01:22:20] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--" "--quiet"
[01:22:20] 
[01:22:20] 
[01:22:20] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:22:20] Build completed unsuccessfully in 0:19:20
[01:22:20] Build completed unsuccessfully in 0:19:20
[01:22:20] make: *** [check] Error 1
[01:22:20] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:02ef8918
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Jan 30 00:40:34 UTC 2019
---
travis_time:end:043f888a:start=1548808835951465266,finish=1548808835956618014,duration=5152748
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:159505c8
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:03caaf58
travis_time:start:03caaf58
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:196aa5c0
$ 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)

bors added a commit that referenced this pull request Jan 30, 2019

Auto merge of #57982 - Centril:rollup, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #57106 (mark str::string::String.trim.* functions as #[must_use].)
 - #57920 (use `SOURCE_DATE_EPOCH` for man page time if set)
 - #57934 (Introduce into_raw_non_null on Rc and Arc)
 - #57974 (override `VecDeque`'s `Iter::try_fold`)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 30, 2019

Failed in rollup, #57982 (comment).

@bors r-

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 30, 2019

Oops, sorry about that. It was very late here when I added the tests.

@llogiq llogiq force-pushed the llogiq:vec-deque-try-fold branch from 7e22cb2 to b472a1a Jan 30, 2019

@llogiq llogiq force-pushed the llogiq:vec-deque-try-fold branch from b472a1a to b062b75 Jan 30, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2019

📌 Commit b062b75 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2019

⌛️ Testing commit b062b75 with merge 147311c...

bors added a commit that referenced this pull request Jan 30, 2019

Auto merge of #57974 - llogiq:vec-deque-try-fold, r=alexcrichton
override `VecDeque`'s `Iter::try_fold`

This should improve performance (wherever it is used), but I haven't found the time to benchmark it yet.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 147311c to master...

@bors bors merged commit b062b75 into rust-lang:master Jan 30, 2019

1 check passed

homu Test successful
Details
@scottmcm
Copy link
Member

scottmcm left a comment

Thanks for picking this up, @llogiq!

{
let (front, back) = RingSlices::ring_slices(self.ring, self.head, self.tail);
let accum = front.iter().try_fold(init, &mut f)?;
back.iter().try_fold(accum, &mut f)

This comment has been minimized.

@scottmcm

scottmcm Jan 30, 2019

Member

I'm concerned about this -- if it short-circuits it's not going to resume from the correct place, since it's try_folding on slice iterators and not updating the state of the self iterator.

This comment has been minimized.

@scottmcm

scottmcm Jan 31, 2019

Member

I'd like to see a test like this one:

rust/src/libcore/tests/iter.rs

Lines 1990 to 1993 in 7164a9f

let a = [10, 20, 30, 40, 100, 60, 70, 80, 90];
let mut iter = a.iter().rev();
assert_eq!(iter.try_fold(0_i8, |acc, &x| acc.checked_add(x)), None);
assert_eq!(iter.next(), Some(&70));

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jan 30, 2019

Oh, oops, apparently I'm late to this :/

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 30, 2019

Oh, oops, apparently I'm late to this :/

There's plenty of time to revert if that's necessary. :)

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 31, 2019

You're right, @scottmcm. I'll do a follow-up PR to rectify this.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jan 31, 2019

Thanks, @llogiq!

(As an aside, while you're at it, consider overriding try_rfold too.)

@llogiq llogiq deleted the llogiq:vec-deque-try-fold branch Jan 31, 2019

@llogiq

This comment has been minimized.

Copy link
Contributor Author

llogiq commented Jan 31, 2019

I'm a bit unsure how to best proceed: Should I try to reinstate the pointer from the slice after the iteration? Or better change the iterator wholesale to a Chain-Wrapper?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment