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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unstable VecDeque::rotate_{left|right} #56842

Merged
merged 3 commits into from Dec 22, 2018

Conversation

Projects
None yet
7 participants
@scottmcm
Copy link
Member

scottmcm commented Dec 15, 2018

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: #56686

馃挘 Please someone look very carefully at the unsafe in this! The wrap_copy seems to be exactly what this method needs, and the len passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘 I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 15, 2018

r? @dtolnay

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

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 15, 2018

This does not hit the #30459 RTL naming issue right?

The implementation looks good to me. r? @alexcrichton for another look at the unsafe code.

r? @alexcrichton

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Dec 16, 2018

I believe this doesn't hit the #30459 RTL issue because it's not on text, and thus is like the rotate_left/rotate_right on integers and slices.

I've pushed a new commit with a claim for why this is safe; please double-check it.

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Dec 16, 2018

VedDeque is front to back in Rust so for consistency with the rest of the API these methods should be named something like rotate_forward or rotate_to_front. VedDeque has no concept of a left or a right. This is in contrast to Python deque which does use left to right.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 17, 2018

I'm not super familiar with the implementation of VecDeque, but this seems fine to me to at least land unstable. I'd ideally prefer to see more tests though especially as this has some unsafety, it looks like there's already internal branches but it may not be covered by tests so far? (things like zero len, over half, under half, etc).

It's true that rotate_left does match integers, but it seems prudent to try think of names at least which match the front/back naming @ollie27 mentioned that's already with VecDeque. Something like rotate_to_front and rotate_to_back would make sense to me too

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Dec 17, 2018

I'll add some more tests.

Since [T]::rotate_left has been stable since 1.26, I think consistency with that is more important than "frontward" names. See previous discussion of the rotate->rotate_left rename in #41891 (comment)

@CryZe

This comment has been minimized.

Copy link
Contributor

CryZe commented Dec 17, 2018

rotate_to_front and rotate_to_back would be ambiguous anyway. The problem here is your viewpoint. Rotating to the back moves all the elements inside the VecDeque from the front to the back, but the ones that fall out of the back get moved to the front. So they traveled from the back to the front, even though the method name suggested we move from the front to the back. That's because rotating from point A to point B on a circle can be done either clockwise or counter clockwise. So from_a_to_b is ambiguous. So the naming would either have to be clockwise / counter_clockwise or left / right to not be ambiguous.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 17, 2018

Ah if rotate_{left,right} are already on slices then makes sense to me to leave the names as-is on VecDeque!

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 19, 2018

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:069c2ffa:start=1545208804155249399,finish=1545208862881547784,duration=58726298385
$ 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
---

[00:03:23] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:23] tidy error: /checkout/src/liballoc/tests/vec_deque.rs: missing trailing newline
[00:03:25] some tidy checks failed
[00:03:25] 
[00:03:25] 
[00:03:25] 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" "--quiet"
[00:03:25] 
[00:03:25] 
[00:03:25] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:25] Build completed unsuccessfully in 0:00:46
[00:03:25] Build completed unsuccessfully in 0:00:46
[00:03:25] Makefile:79: recipe for target 'tidy' failed
[00:03:25] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:056d282c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 19 08:44:36 UTC 2018
---
travis_time:end:2f39d1fc:start=1545209077408739922,finish=1545209077414535688,duration=5795766
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1a330803
$ 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:0b815371
travis_time:start:0b815371
$ 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:0425b0d4
$ 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)

@scottmcm scottmcm force-pushed the scottmcm:vecdeque-rotate branch from d58b37b to cbe9abb Dec 19, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 19, 2018

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

馃搶 Commit cbe9abb has been approved by alexcrichton

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 20, 2018

Rollup merge of rust-lang#56842 - scottmcm:vecdeque-rotate, r=alexcri鈥
鈥hton

Add unstable VecDeque::rotate_{left|right}

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: rust-lang#56686

~~馃挘 Please someone look very carefully at the `unsafe` in this!  The `wrap_copy` seems to be exactly what this method needs, and the `len` passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘~~ I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.

bors added a commit that referenced this pull request Dec 20, 2018

Auto merge of #57005 - pietroalbini:rollup, r=pietroalbini
Rollup of 19 pull requests

Successful merges:

 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56843 (Add a note describing the type of the non-Copy moved variable)
 - #56845 (Don't render const keyword on stable)
 - #56862 (stop treating trait objects from #[fundamental] traits as fundamental)
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56906 (Issue #56905)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56916 (Fix a recently introduced regression)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56954 (Add dist builder for Armv8-M Mainline)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 20, 2018

鈱涳笍 Testing commit cbe9abb with merge 04cfa08...

bors added a commit that referenced this pull request Dec 20, 2018

Auto merge of #56842 - scottmcm:vecdeque-rotate, r=alexcrichton
Add unstable VecDeque::rotate_{left|right}

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: #56686

~~馃挘 Please someone look very carefully at the `unsafe` in this!  The `wrap_copy` seems to be exactly what this method needs, and the `len` passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘~~ I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 20, 2018

馃挃 Test failed - status-travis

@bors bors removed the S-waiting-on-bors label Dec 20, 2018

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 20, 2018

The job x86_64-gnu-distcheck 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.
[00:03:25]    Compiling cargo v0.32.0
[00:03:32] error[E0308]: mismatched types
[00:03:32]  --> /cargo/registry/src/github.com-1ecc6299db9ec823/cargo-0.32.0/src/cargo/util/sha256.rs:9:34
[00:03:32]   |
[00:03:32] 9 |         let hasher = Hasher::new(Algorithm::SHA256);
[00:03:32]   |                                  |
[00:03:32]   |                                  |
[00:03:32]   |                                  expected reference, found enum `util::sha256::crypto_hash::Algorithm`
[00:03:32]   |                                  help: consider borrowing here: `&Algorithm::SHA256`
[00:03:32]   |
[00:03:32]   = note: expected type `&util::sha256::crypto_hash::Algorithm`
[00:03:32]              found type `util::sha256::crypto_hash::Algorithm`
[00:03:32] error: aborting due to previous error
[00:03:32] 
[00:03:32] For more information about this error, try `rustc --explain E0308`.
[00:03:32] For more information about this error, try `rustc --explain E0308`.
[00:03:32] error: failed to compile `cargo-vendor v0.1.22`, intermediate artifacts can be found at `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools`
[00:03:32] Caused by:
[00:03:32]   Could not compile `cargo`.
[00:03:32] 
[00:03:32] To learn more, run the command again with --verbose.
[00:03:32] To learn more, run the command again with --verbose.
[00:03:32] 
[00:03:32] 
[00:03:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "install" "-j" "4" "--locked" "--color" "always" "--force" "--debug" "--vers" "0.1.22" "cargo-vendor"
[00:03:32] 
[00:03:32] 
[00:03:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test distcheck
[00:03:32] Build completed unsuccessfully in 0:01:37
---
travis_time:end:011d77f4:start=1545333368872761108,finish=1545333368880427904,duration=7666796
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0357c490
$ 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:0d6db535
travis_time:start:0d6db535
$ 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:092ec13a
$ 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)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 20, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 21, 2018

Rollup merge of rust-lang#56842 - scottmcm:vecdeque-rotate, r=alexcri鈥
鈥hton

Add unstable VecDeque::rotate_{left|right}

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: rust-lang#56686

~~馃挘 Please someone look very carefully at the `unsafe` in this!  The `wrap_copy` seems to be exactly what this method needs, and the `len` passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘~~ I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.

bors added a commit that referenced this pull request Dec 21, 2018

Auto merge of #57025 - pietroalbini:rollup, r=pietroalbini
Rollup of 21 pull requests

Successful merges:

 - #56802 (Add DoubleEndedIterator::nth_back)
 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56917 (Simplify MIR generation for logical operations)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56941 (deny intra-doc link resolution failures in libstd)
 - #56954 (Add dist builder for Armv8-M Mainline)
 - #56964 (Remove `TokenStream::JointTree`.)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56981 (miri: allocation is infallible)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)
 - #56992 (suggest similar lint names for unknown lints)
 - #57002 (Stabilize Vec(Deque)::resize_with)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Dec 21, 2018

Auto merge of #57025 - pietroalbini:rollup, r=pietroalbini
Rollup of 20 pull requests

Successful merges:

 - #56802 (Add DoubleEndedIterator::nth_back)
 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56917 (Simplify MIR generation for logical operations)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56941 (deny intra-doc link resolution failures in libstd)
 - #56964 (Remove `TokenStream::JointTree`.)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56981 (miri: allocation is infallible)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)
 - #56992 (suggest similar lint names for unknown lints)
 - #57002 (Stabilize Vec(Deque)::resize_with)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Dec 21, 2018

Auto merge of #57025 - pietroalbini:rollup, r=pietroalbini
Rollup of 20 pull requests

Successful merges:

 - #56802 (Add DoubleEndedIterator::nth_back)
 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56917 (Simplify MIR generation for logical operations)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56941 (deny intra-doc link resolution failures in libstd)
 - #56964 (Remove `TokenStream::JointTree`.)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56981 (miri: allocation is infallible)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)
 - #56992 (suggest similar lint names for unknown lints)
 - #57002 (Stabilize Vec(Deque)::resize_with)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2018

Rollup merge of rust-lang#56842 - scottmcm:vecdeque-rotate, r=alexcri鈥
鈥hton

Add unstable VecDeque::rotate_{left|right}

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: rust-lang#56686

~~馃挘 Please someone look very carefully at the `unsafe` in this!  The `wrap_copy` seems to be exactly what this method needs, and the `len` passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘~~ I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2018

鈱涳笍 Testing commit cbe9abb with merge 9689ada...

bors added a commit that referenced this pull request Dec 22, 2018

Auto merge of #56842 - scottmcm:vecdeque-rotate, r=alexcrichton
Add unstable VecDeque::rotate_{left|right}

Like the ones on slices, but more efficient because vecdeque is a circular buffer.

Issue that proposed this: #56686

~~馃挘 Please someone look very carefully at the `unsafe` in this!  The `wrap_copy` seems to be exactly what this method needs, and the `len` passed to it is never more than half the length of the deque, but I haven't managed to prove to myself that it's correct 馃挘~~ I think I proved that this code meets the requirement of the unsafe code it's calling; please double-check, of course.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2018

鈽锔 Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9689ada to master...

@bors bors merged commit cbe9abb into rust-lang:master Dec 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@scottmcm scottmcm deleted the scottmcm:vecdeque-rotate branch Jan 14, 2019

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