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

Tracking issue for copy_within #54236

Closed
4 tasks done
oconnor663 opened this issue Sep 14, 2018 · 14 comments · Fixed by #61398
Closed
4 tasks done

Tracking issue for copy_within #54236

oconnor663 opened this issue Sep 14, 2018 · 14 comments · Fixed by #61398
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@oconnor663
Copy link
Contributor

oconnor663 commented Sep 14, 2018

Rust PR: #53652
Standalone implementation: https://github.com/oconnor663/copy_in_place

Summary: 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.

Open questions: @SimonSapin had a proposal for an alternative API using ranges. Here's an example PR along those lines: oconnor663/copy_in_place#1

@Centril Centril added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Sep 14, 2018
@oconnor663 oconnor663 changed the title Tracking issue for copy_in_place Tracking issue for copy_within Sep 18, 2018
@oconnor663
Copy link
Contributor Author

@alexcrichton what do the next steps look like for something like this? How do we "decide" on an API? :)

@alexcrichton
Copy link
Member

@oconnor663 the process here is somewhat informal, but after the API has been in nightly for some time it's renominated for stabilization with any open questions, and at that point the libs team all signs off on the stabilization.

To that end it's basically "let this sit for awhile and gain feedback", and then after awhile has passed it can be proposed to be stabilized

@alexcrichton alexcrichton added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Sep 24, 2018
oconnor663 added a commit to oconnor663/bao that referenced this issue Sep 24, 2018
We won't be able to land this in master for probably several major Rust
versions, but it's nice to see it working. Tracking issue:
rust-lang/rust#54236
@briansmith
Copy link
Contributor

FWIW, I tried this out in ring and here's my feedback:

  • I wish there was a non-panicking version that failed with an error result when the bounds are out of range. I ended up having to duplicate the range checking in my own code.

  • It is very important that the implementation be inlined but the implementation doesn't have #[inline]. In practice the compiler was always inlining it for me but it would be good to do something more to ensure this.

@oconnor663
Copy link
Contributor Author

Thanks for trying it out.

  1. What's the situation where you need to check against making an out of bounds copy? Would it be possible to see the code where this came up? What fallback would you be using if copy_within returned an error?

  2. make copy_within #[inline] #56886

@briansmith
Copy link
Contributor

What's the situation where you need to check against making an out of bounds copy? Would it be possible to see the code where this came up? What fallback would you be using if copy_within returned an error?

I have my own API that is like copy_within, but which does an encryption/decryption operation as part of the copying, but my API avoids panicking on invalid bounds. There wouldn't be any fallback used; I just return an error.

@oconnor663
Copy link
Contributor Author

I don't imagine it would make sense to hoist the bounds checking requirement up to the caller, and just have ring propagate the panic if the bounds are wrong? I guess the question is whether any of the callers are expected to have a fallback, or whether bad bounds are more likely to be programmer error.

@briansmith
Copy link
Contributor

I don't imagine it would make sense to hoist the bounds checking requirement up to the caller, and just have ring propagate the panic if the bounds are wrong? I guess the question is whether any of the callers are expected to have a fallback, or whether bad bounds are more likely to be programmer error.

There are lots of possibilities. This isn't the only API in libstd that has only a panicking version so if that's what libstd people like, I would just carry on as-is.

@oconnor663
Copy link
Contributor Author

Yeah I was definitely working by analogy with copy_from_slice and friends.

@Shnatsel
Copy link
Member

Shnatsel commented Jan 4, 2019

FWIW this is desired by Ropey crate. Currently unsafe code is used to achieve this.

@SimonSapin
Copy link
Contributor

@briansmith Adding #[inline] sounds good, do you want to send a PR?

The arguments for a non-panicking version also sound good to me. Perhaps even instead of, not in addition to what’s implemented today. And users that want panic can call unwrap.

What API should this have? Perhaps a Result return type rather than Option to take advantage of #[must_use], but there is no precedent in std for Result<…, ()>, so with a new dedicated error type?

The other API that does non-panicky bounds checking of a range argument is [T]::get which returns an Option, but that one doesn’t have side effects and is only useful for its return value.

@oconnor663
Copy link
Contributor Author

@SimonSapin I put an #[inline] change up as #56886, but @sfackler was opposed to it. There's some discussion in the PR.

I feel like I remember some General Guidance from many years ago, when there was a big cleanup going on in the stdlib, about when to have panicking vs option/result APIs or when to have both. Does anyone remember where that was or what the consensus was there?

@oconnor663
Copy link
Contributor Author

@Shnatsel this function is available in the tiny copy_in_place crate, if you want to keep the unsafe code out of the caller.

@joshlf
Copy link
Contributor

joshlf commented Mar 9, 2019

To that end it's basically "let this sit for awhile and gain feedback", and then after awhile has passed it can be proposed to be stabilized

Has it been a while yet? :) I'd like to use this if stabilized.

@kennytm
Copy link
Member

kennytm commented May 31, 2019

Filed a stabilization PR at #61398.

Centril added a commit to Centril/rust that referenced this issue Jun 12, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants