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

Stabilize mutable slice API #17494

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@aturon
Copy link
Member

aturon commented Sep 23, 2014

This commit is another in the series of vector slice API stabilization. The focus here is the mutable slice API.

Largely, this API inherits the stability attributes previously assigned to the analogous methods on immutable slides. It also adds comments to a few unstable attributes that were previously missing them.

In addition, the commit adds several _mut variants of APIs that were missing:

  • init_mut
  • head_mut
  • tail_mut
  • splitn_mut
  • rsplitn_mut

Some of the unsafe APIs -- unsafe_set, init_elem, and copy_memory -- were deprecated in favor of working through as_mut_ptr, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 23, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@aturon aturon force-pushed the aturon:stabilize-mutable-slices branch from 2db3c9d to 5307857 Sep 23, 2014

@@ -778,10 +801,11 @@ pub trait MutableSlice<'a, T> {
/// // v.unsafe_set(10, "oops".to_string());
/// }
/// ```
#[deprecated = "use as_mut_ptr"]

This comment has been minimized.

@alexcrichton

alexcrichton Sep 24, 2014

Member

This may also want to mention the set part as well:

#[deprecated = "use `*foo.as_mut_ptr().offset(index) = val`"]
@@ -792,6 +816,7 @@ pub trait MutableSlice<'a, T> {
/// // memory leak! `"bar".to_string()` is not deallocated.
/// unsafe { v.init_elem(1, "baz".to_string()); }
/// ```
#[deprecated = "use as_mut_ptr"]
unsafe fn init_elem(self, i: uint, val: T);

This comment has been minimized.

@alexcrichton

alexcrichton Sep 24, 2014

Member
#[deprecated = "use `ptr::write(foo.as_mut_ptr().offset(i), val)`"]
@@ -799,6 +824,7 @@ pub trait MutableSlice<'a, T> {
/// This does not run destructors on the overwritten elements, and
/// ignores move semantics. `self` and `src` must not
/// overlap. Fails if `self` is shorter than `src`.
#[deprecated = "use as_mut_ptr"]
unsafe fn copy_memory(self, src: &[T]);

This comment has been minimized.

@alexcrichton

alexcrichton Sep 24, 2014

Member
#[deprecated = "use as_mut_ptr and ptr::copy_memory"]
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 24, 2014

Looks good to me! r=me with some adjustments to the deprecation messages.

@aturon aturon force-pushed the aturon:stabilize-mutable-slices branch from 5307857 to 9834d45 Sep 24, 2014

bors added a commit that referenced this pull request Sep 25, 2014

auto merge of #17494 : aturon/rust/stabilize-mutable-slices, r=alexcr…
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

bors added a commit that referenced this pull request Sep 25, 2014

auto merge of #17494 : aturon/rust/stabilize-mutable-slices, r=alexcr…
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

bors added a commit that referenced this pull request Sep 25, 2014

auto merge of #17494 : aturon/rust/stabilize-mutable-slices, r=alexcr…
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

bors added a commit that referenced this pull request Sep 25, 2014

auto merge of #17494 : aturon/rust/stabilize-mutable-slices, r=alexcr…
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

aturon added some commits Sep 22, 2014

Stabilize mutable slice API
This commit is another in the series of vector slice API
stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously
assigned](#16332) to the analogous
methods on immutable slides. It also adds comments to a few `unstable`
attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were
missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory`
-- were deprecated in favor of working through `as_mut_ptr`, to simplify
the API surface.

Due to deprecations, this is a:

[breaking-change]

@aturon aturon force-pushed the aturon:stabilize-mutable-slices branch from 9834d45 to c59ef66 Sep 26, 2014

@aturon

This comment has been minimized.

Copy link
Owner Author

aturon commented on c59ef66 Sep 26, 2014

r=alexcrichton

@thestinger thestinger closed this Sep 26, 2014

@thestinger thestinger reopened this Sep 26, 2014

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Sep 26, 2014

Why is this a regression for low-level Rust?

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Sep 26, 2014

This is a small change that affects two methods that are rarely used in the extant codebase. Nobody felt that it would be controversial and the implication that we were deliberately attempting to sidestep community consensus is incorrect.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 26, 2014

@thestinger Can you be more specific about how this is making unchecked indexing on slices harder?

Discussion about this is happening now.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 26, 2014

@thestinger Do you have specific criticisms of the content of this pull request that you would like to discuss?

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Sep 26, 2014

I think the problem here is just a poor deprecation message. It should instead suggest:

Replace v.unsafe_set(i, x) with *v.unsafe_mut(i) = x.

/// }
/// ```
/// Deprecated: use `*foo.as_mut_ptr().offset(index) = val` instead.
#[deprecated = "use `*foo.as_mut_ptr().offset(index) = val`"]

This comment has been minimized.

@Gankro

Gankro Sep 26, 2014

Contributor

Isn't *foo.unsafe_mut(index) = val a better choice here?

Edit: ah, this was already stated elsewhere.

This comment has been minimized.

@aturon

aturon Sep 26, 2014

Author Member

Yes -- see comment in the github thread above. This was just a mistake in the message.

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Sep 26, 2014

init_elem and copy_memory are clearly more specialized operations than skipping a bounds check.

As with many other deprecations (of safe APIs), these were eliminated as conveniences that weren't worth the extra API surface area. There's also a general push toward funneling conveniences through central traits/data structures, like Iterator and in this case ptr. The benefit is that conveniences added to these central structures have a multiplicative effect, and it's much easier to remember a single API surface rather than ad hoc convenience methods spread through different types.

If you want to do extensive unsafe work with slices, the simplest avenue would be to use .as_mut_ptr() once, and then apply as many operations as you like to the resulting pointer. And of course there's always the option to add extension traits providing any conveniences you like.

It's also worth noting that deprecations like these don't entail never providing such conveniences in the future, if they are ultimately deemed worthwhile. But there has been a lot of accumulation of ad hoc APIs, and the stabilization process is trying to move things toward a somewhat minimalistic, consistent, coherent, clean, and reasonably ergonomic state heading toward 1.0. (For bigger changes, like conventions, or removing the collection traits, or consolidating the numerics hierarchy, this involves RFCs.)

As an aside, we should consider adding an Index implementation to raw pointers, which would recover ergonomics very similar to C's -- and all by funneling through raw pointers.

@thestinger

This comment has been minimized.

Copy link
Contributor

thestinger commented Sep 26, 2014

I still think it's a regression and I disagree with this process but I'm not interested in arguing about it anymore. I don't care enough about the standard library to deal with that.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on c59ef66 Sep 26, 2014

saw approval from alexcrichton
at aturon@c59ef66

This comment has been minimized.

Copy link
Contributor

bors replied Sep 26, 2014

merging aturon/rust/stabilize-mutable-slices = c59ef66 into auto

This comment has been minimized.

Copy link
Contributor

bors replied Sep 26, 2014

aturon/rust/stabilize-mutable-slices = c59ef66 merged ok, testing candidate = 5d653c1

This comment has been minimized.

Copy link
Contributor

bors replied Sep 26, 2014

fast-forwarding master to auto = 5d653c1

bors added a commit that referenced this pull request Sep 26, 2014

auto merge of #17494 : aturon/rust/stabilize-mutable-slices, r=alexcr…
…ichton

This commit is another in the series of vector slice API stabilization. The focus here is the *mutable* slice API.

Largely, this API inherits the stability attributes [previously assigned](#16332) to the analogous methods on immutable slides. It also adds comments to a few `unstable` attributes that were previously missing them.

In addition, the commit adds several `_mut` variants of APIs that were missing:

- `init_mut`
- `head_mut`
- `tail_mut`
- `splitn_mut`
- `rsplitn_mut`

Some of the unsafe APIs -- `unsafe_set`, `init_elem`, and `copy_memory` -- were deprecated in favor of working through `as_mut_ptr`, to simplify the API surface.

Due to deprecations, this is a:

[breaking-change]

@bors bors closed this Sep 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.