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

define copy_within on slices #53652

Merged
merged 2 commits into from
Sep 22, 2018
Merged

define copy_within on slices #53652

merged 2 commits into from
Sep 22, 2018

Conversation

oconnor663
Copy link
Contributor

This is a safe wrapper around ptr::copy, for regions within a single slice. Previously, safe in-place copying was only available as a side effect of Vec::drain.

I've wanted this API a couple times in the past, and I figured I'd just whip up a PR to help discuss it. It's possible something like this exists elsewhere and I just missed it. It might also be a big enough addition to warrant an RFC, I'm not sure.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.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.
[00:47:45] ....................................................................................................
[00:47:49] ....................................................................................................
[00:47:51] .........i..........................................................................................
[00:47:54] ....................................................................................................
[00:47:57] ..........................................................iiiiiiiii.................................
[00:48:02] ....................................................................................................
[00:48:06] ....................................................................................................
[00:48:09] .......................................i............................................................
[00:48:12] .........................................................................................i.i..ii....
---
[01:05:33] ....................................................................................................
[01:05:39] ....................................................................................................
[01:05:47] ....................................................................................................
[01:05:54] ....................................................................................................
[01:06:02] ...................................................................F................................
[01:06:16] failures:
[01:06:16] 
[01:06:16] ---- slice/mod.rs - slice::[T]::copy_in_place (line 1640) stdout ----
[01:06:16] ---- slice/mod.rs - slice::[T]::copy_in_place (line 1640) stdout ----
[01:06:16] error[E0658]: use of unstable library feature 'copy_in_place' (see issue #53652)
[01:06:16]  --> slice/mod.rs:1643:7
[01:06:16]   |
[01:06:16] 6 | slice.copy_in_place(1, 0, 2);
[01:06:16]   |
[01:06:16]   |
[01:06:16]   = help: add #![feature(copy_in_place)] to the crate attributes to enable
[01:06:16] thread 'slice/mod.rs - slice::[T]::copy_in_place (line 1640)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:06:16] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:06:16] 
[01:06:16] 
---
[01:06:16] 
[01:06:16] error: test failed, to rerun pass '--doc'
[01:06:16] 
[01:06:16] 
[01:06:16] 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 jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:06:16] 
[01:06:16] 
[01:06:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:06:16] Build completed unsuccessfully in 0:22:34
[01:06:16] Build completed unsuccessfully in 0:22:34
[01:06:16] make: *** [check] Error 1
travis_time:end:00b5a7e0:start=1535068081050544893,finish=1535072057765353580,duration=3976714808687

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:000ec932

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)

@oconnor663
Copy link
Contributor Author

I added /// # #![feature(copy_in_place)] to the doc example to hopefully fix that build break, and I tweaked the example itself to hopefully be a little clearer.

@BurntPizza
Copy link
Contributor

Seems like it needs get_unchecked* to get rid of the bounds checks despite the asserts, unless I'm missing something and that's not safe: https://godbolt.org/z/W3m1kv

@oconnor663
Copy link
Contributor Author

@BurntPizza thanks for catching that. I've amended and rebased.

@oconnor663
Copy link
Contributor Author

oconnor663 commented Aug 24, 2018

Safety question not necessarily specific to this PR: Indexing into a slice relies on ptr::offset, which says that its UB to use an offset/index larger than isize::MAX. As documented by ptr::offset, "memory acquired directly from allocators or memory mapped files may be too large to handle with this function." Does that mean that memory mapping a file larger than isize::MAX is unsound, because safe code can cause UB just by indexing into it? (Edit: I split this out into #53676.)

@oconnor663
Copy link
Contributor Author

I've published this function as a standalone crate in the meantime: https://crates.io/crates/copy_in_place

@TimNN
Copy link
Contributor

TimNN commented Sep 4, 2018

Ping from triage @Kimundi / @rust-lang/libs: This PR requires your review.

@alexcrichton
Copy link
Member

Thanks for the PR! The API here looks pretty good to me, although I might suggest copy_within perhaps to bikeshed the name? Other than that though with a tracking issue I think this should be good to go

@SimonSapin
Copy link
Contributor

bytes.copy_in_place(1, 8, 4)

With three usize arguments I feel like I’d need to look up the docs every time in order to figure out which argument is which and what this call does. Maybe range syntax could make calls clearer?

pub fn copy_in_place(&mut self, src: Range<usize>, dest: RangeFrom<usize>) where T: Copy {
   assert!(src.end >= src.start);
   let count = src.end - src.start;
   // …
}
bytes.copy_in_place(1..5, 8..)

@oconnor663
Copy link
Contributor Author

oconnor663 commented Sep 6, 2018

The name: I noticed there was some precedent along the lines of drop_in_place and grow_in_place in the standard library. Otherwise I don't have any strong feelings.

The API: Agreed that it's hard to remember. I think the one major advantage that the current approach has going for it is that it's a pretty direct clone of ptr::copy(src, dest, count). I didn't think about using ranges before, but I wonder if that would lead users to expect that other types of ranges would work. If not, it could be a little frustrating. But if so, it could lead to other types of confusion, for example slice.copy_in_place(i.., j..) (the first presumably ending at the end of the slice, and the second then taking the length of the first, which isn't clear just from looking at it). One of the design thoughts I had, which you preserved, was to leave no room in the API to specify regions of different sizes, and so no need to check that particular condition and panic. But if we were willing to relax that, we could say template on any arg types implementing std::ops::RangeBounds. Upsides: Very easy to remember how to call. Downsides: Noisier to read if the caller is starting with a count and needs to compute both range ends. Also possibly more overhead if it's being called inside an exceptionally tight loop.

Tracking issue: @alexcrichton how does that work? Would I file one, or would someone on the core team? Before or after this PR lands?

@oconnor663
Copy link
Contributor Author

oconnor663 commented Sep 6, 2018

I guess another option would be to define some kind of arg struct, so that the caller is forced to name each field that it's passing in. That could help readability, but I don't know if there's precedent for it in the standard library. It would also be unnecessarily verbose in the case where the caller already has variables with decent names.

Or perhaps the src and dest args could live in a tuple, just to set them apart? That would be kind of quirky though.

@SimonSapin
Copy link
Contributor

it's a pretty direct clone of ptr::copy(src, dest, count)

But ptr::copy has different types of arguments (two pointers and an integer), which conveys information about which argument is which. Compare copy(a.as_ptr(), b.as_mut_ptr(), 4) v.s. copy_in_place(1, 8, 4).

Regarding a generic parameter with RangeBounds, maybe only for src?

@oconnor663
Copy link
Contributor Author

oconnor663 commented Sep 10, 2018

I opened a PR on my crate implementation to take a look at what a range argument would look like. It seems viable. The upside is that the argument type's a lot more visible. The downside is that a caller with a count instead of a src_end needs to do one extra math step before calling (though this counts as an upside too if you already have a src_end, so maybe a wash). Also there's one more way to panic, if you supply an inverted range.

@oconnor663
Copy link
Contributor Author

@alexcrichton could you say more about how tracking issues work? Should I file one now, or wait until we have a little more consensus on the API here?

@alexcrichton
Copy link
Member

@oconnor663 oh sure yeah, so for us a tracking issue is created whenever we merge an unstable feature. So to land this PR we'll want a tracking issue which lists out the various design questions and such.

@oconnor663
Copy link
Contributor Author

oconnor663 commented Sep 14, 2018

I've opened a tracking issue (#54236), updated the issue number referenced inline in this PR, and rebased.

assert!(dest <= self.len() - count);
unsafe {
let src_ptr = self.get_unchecked(src) as *const T;
let dest_ptr = self.get_unchecked_mut(dest) as *mut T;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You shouldn't need to cast these; they'll coerce just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think without the casts this fails the borrow checker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, you're of course right. I was thinking of when they're not in variables:

        ptr::copy(
            slice.get_unchecked(src),
            slice.get_unchecked_mut(dest),
            count,
        );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gotcha. Do you think that version would be better style? I'd be happy with either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion, other than that I do like avoiding as, especially to *mut.

Maybe keep the variables, but coerce instead of casting?

            let src_ptr: *const T = self.get_unchecked(src);
            let dest_ptr: *mut T = self.get_unchecked_mut(dest);

@scottmcm
Copy link
Member

Previously, safe in-place copying was only available as a side effect of Vec::drain.

Would we only suggest this when things can overlap? Or would this replace using split_at_mut when the sides cannot overlap? If I'm reading it right, the one example in the docs doesn't need this method:

let (a, b) = bytes.split_at_mut(8);
b[..4].copy_from_slice(a[1..5]);

@oconnor663
Copy link
Contributor Author

Would we only suggest this when things can overlap?

I'm not sure. On the one hand, the two-liner above would become a one-liner:

bytes.copy_in_place(9, 0, 4);

// Or with the API SimonSapin suggested:
bytes.copy_in_place(9..13, 0);

On the other hand, I think copy_from_slice gets turned into a ptr::copy_nonoverlapping, which might have slightly better performance than ptr::copy. Is that a meaningful difference, in the disjoint case where both approaches are possible? Maybe someone who knows more about those perf details can weigh in.

@oconnor663
Copy link
Contributor Author

I've gotten rid of pointer casts as @scottmcm suggested, switched to @SimonSapin's suggested API, and rebased again.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.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.

[00:06:27] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:06:27] tidy error: /checkout/src/libcore/slice/mod.rs:1647: line longer than 100 chars
[00:06:27] tidy error: /checkout/src/libcore/slice/mod.rs:1650: line longer than 100 chars
[00:06:27] tidy error: /checkout/src/libcore/slice/mod.rs:1654: line longer than 100 chars
[00:06:28] some tidy checks failed
[00:06:28] 
[00:06:28] 
[00:06:28] 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:06:28] 
[00:06:28] 
[00:06:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:06:28] Build completed unsuccessfully in 0:00:49
[00:06:28] Build completed unsuccessfully in 0:00:49
[00:06:28] Makefile:79: recipe for target 'tidy' failed
[00:06:28] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:10bc0222
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:004e1dac:start=1537219812203413386,finish=1537219812208174642,duration=4761256
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:19b7bab0
$ 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 -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:242c3f78
travis_time:start:242c3f78
$ 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:12fe7975
$ 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
Copy link
Member

Looks good to me! I think there's some tidy/style issues though?

@oconnor663
Copy link
Contributor Author

Line length should be fixed now.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.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.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/f8/ab/ab7b15a7a5524f47bb39279a59a7afdb1237162159ba7ff15cab28c96915/awscli-1.16.15-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 11.5MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▊                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01
---
[00:57:25] ....................................................................................................
[00:57:28] .....................................................i..............................................
[00:57:31] ....................................................................................................
[00:57:34] ....................................................................................................
[00:57:37] .iiiiiiiii..........................................................................................
[00:57:42] ....................................................................................................
[00:57:45] ..................................................................................i.................
[00:57:48] ....................................................................................................
[00:57:51] ....................................i.i..ii.........................................................
---
[01:16:26] .................................................................................................... 1600/2163
[01:16:32] .................................................................................................... 1700/2163
[01:16:40] .................................................................................................... 1800/2163
[01:16:47] .................................................................................................... 1900/2163
[01:16:55] ........................................................................F........................... 2000/2163
[01:17:06] ....................................................................................i............... 2100/2163
mental/bootstrap-14ucxxomeo8gg/s-f4wkbzrx95-1hlrm69-1c0ivx6vup97q
131204 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
131200 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
129780 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu
129776 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release

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)

@oconnor663
Copy link
Contributor Author

Very silly copy-paste-o on my part. Should be fixed. I also decided I like @alexcrichton's name suggestion copy_within more, so I'm going with that for now.

@oconnor663 oconnor663 changed the title define copy_in_place on slices define copy_within on slices Sep 18, 2018
@alexcrichton
Copy link
Member

Thanks! Sorry I forgot to mention earlier, but could some semi-exhaustive tests be added for this as well? With unsafe in play it's always good to have a good suite of tests to handle issues!

This is a safe wrapper around ptr::copy, for regions within a single
slice. Previously, safe in-place copying was only available as a side
effect of Vec::drain.
@oconnor663
Copy link
Contributor Author

I've added some tests, which are hopefully about to pass Travis. I could add more?

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Sep 20, 2018

📌 Commit d0e59f5 has been approved by alexcrichton

@bors 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 Sep 20, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Sep 21, 2018
…chton

define copy_within on slices

This is a safe wrapper around `ptr::copy`, for regions within a single slice. Previously, safe in-place copying was only available as a side effect of `Vec::drain`.

I've wanted this API a couple times in the past, and I figured I'd just whip up a PR to help discuss it. It's possible something like this exists elsewhere and I just missed it. It might also be a big enough addition to warrant an RFC, I'm not sure.
bors added a commit that referenced this pull request Sep 21, 2018
Rollup of 10 pull requests

Successful merges:

 - #53652 (define copy_within on slices)
 - #54261 (Make `dyn` a keyword in the 2018 edition)
 - #54317 (Implement the dbg!(..) macro)
 - #54323 (rustbuild: drop color handling)
 - #54371 (add -Zui-testing to rustdoc)
 - #54374 (Make 'proc_macro::MultiSpan' public.)
 - #54402 (Use no_default_libraries for all NetBSD flavors)
 - #54412 (add applicability to span_suggestion call)
 - #54413 (Add UI test for deref recursion limit printing twice)
 - #54422 (Simplify slice's first(_mut) and last(_mut) with get)
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Sep 22, 2018
…chton

define copy_within on slices

This is a safe wrapper around `ptr::copy`, for regions within a single slice. Previously, safe in-place copying was only available as a side effect of `Vec::drain`.

I've wanted this API a couple times in the past, and I figured I'd just whip up a PR to help discuss it. It's possible something like this exists elsewhere and I just missed it. It might also be a big enough addition to warrant an RFC, I'm not sure.
bors added a commit that referenced this pull request Sep 22, 2018
Rollup of 16 pull requests

Successful merges:

 - #53652 (define copy_within on slices)
 - #54261 (Make `dyn` a keyword in the 2018 edition)
 - #54280 (remove (more) CAS API from Atomic* types where not natively supported)
 - #54323 (rustbuild: drop color handling)
 - #54350 (Support specifying edition in doc test)
 - #54370 (Improve handling of type bounds in `bit_set.rs`.)
 - #54371 (add -Zui-testing to rustdoc)
 - #54374 (Make 'proc_macro::MultiSpan' public.)
 - #54402 (Use no_default_libraries for all NetBSD flavors)
 - #54409 (Detect `for _ in in bar {}` typo)
 - #54412 (add applicability to span_suggestion call)
 - #54413 (Add UI test for deref recursion limit printing twice)
 - #54415 (parser: Tweak function parameter parsing to avoid rollback on succesfull path)
 - #54420 (Compress `Liveness` data some more.)
 - #54422 (Simplify slice's first(_mut) and last(_mut) with get)
 - #54446 (Unify christianpoveda's emails)

Failed merges:

 - #54058 (Introduce the partition_dedup/by/by_key methods for slices)

r? @ghost
bors added a commit that referenced this pull request Sep 22, 2018
Rollup of 16 pull requests

Successful merges:

 - #53652 (define copy_within on slices)
 - #54261 (Make `dyn` a keyword in the 2018 edition)
 - #54280 (remove (more) CAS API from Atomic* types where not natively supported)
 - #54323 (rustbuild: drop color handling)
 - #54350 (Support specifying edition in doc test)
 - #54370 (Improve handling of type bounds in `bit_set.rs`.)
 - #54371 (add -Zui-testing to rustdoc)
 - #54374 (Make 'proc_macro::MultiSpan' public.)
 - #54402 (Use no_default_libraries for all NetBSD flavors)
 - #54409 (Detect `for _ in in bar {}` typo)
 - #54412 (add applicability to span_suggestion call)
 - #54413 (Add UI test for deref recursion limit printing twice)
 - #54415 (parser: Tweak function parameter parsing to avoid rollback on succesfull path)
 - #54420 (Compress `Liveness` data some more.)
 - #54422 (Simplify slice's first(_mut) and last(_mut) with get)
 - #54446 (Unify christianpoveda's emails)

Failed merges:

 - #54058 (Introduce the partition_dedup/by/by_key methods for slices)

r? @ghost
@bors bors merged commit d0e59f5 into rust-lang:master Sep 22, 2018
@kennytm kennytm mentioned this pull request May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants