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 the slice::fill method #70758

Closed
3 tasks done
yoshuawuyts opened this issue Apr 4, 2020 · 20 comments
Closed
3 tasks done

Tracking Issue for the slice::fill method #70758

yoshuawuyts opened this issue Apr 4, 2020 · 20 comments
Labels
A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Apr 4, 2020

This is a tracking issue for the slice::fill method.
The feature gate for the issue is #![feature(slice_fill)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Is V: Borrow<T> the most appropriate bound for the argument?

Implementation history

@yoshuawuyts yoshuawuyts added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Apr 4, 2020
@jonas-schievink jonas-schievink added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 4, 2020
@cuviper
Copy link
Member

cuviper commented Apr 5, 2020

Is V: Borrow<T> the most appropriate bound for the argument?

If nothing else, it feels a little weird that this is in reverse of how the maps and sets use Borrow, e.g. K: Borrow<Q> where Q is the argument. But in those cases, they don't need to create keys, whereas fill is using clone_from, so it's a bit different.

@lcnr
Copy link
Contributor

lcnr commented Apr 6, 2020

I don't like the trait bound Borrow<T> as it adds complexity without a clear improvement imo.
Using V instead of &T does not enable code that is otherwise impossible, as V.borrow() returns a &T, so slice::fill(V.borrow()) can be used in this case. Borrow also has additional requirements which are not necessary here:

In particular Eq, Ord and Hash must be equivalent for borrowed and owned values: x.borrow() == y.borrow() should give the same result as x == y.

(std)

@SimonSapin
Copy link
Contributor

+1 for taking a &T parameter

  • Fewer type parameters reduces likelihood of type inference being ambiguous in various programs
  • In this case there is no loss of functionality over slice.fill(v.borrow()), as @lcnr pointed out

A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

This reads like a template meant for "large" language features and doesn’t seem to really apply here. There is not a github label for every single unstable library feature.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Apr 15, 2020

+1 for taking a &T parameter

This does make the common case somewhat awkward:

let mut buf = vec![0; 10];
buf.fill(1);  // compiler error!
buf.fill(&1); // ok
assert_eq!(buf, vec![1; 10]);

Understanding why a Copy value needs to be passed by reference in just this API may confuse people. The rationale behind adding the Borrow bound was to reconcile the common case with the desire to not always take-by-value #70752 (comment), while still enabling the common case of passing by-value:

let mut buf = vec![0; 10];
buf.fill(1);  // ok
buf.fill(&1); // ok
assert_eq!(buf, vec![1; 10]);

An alternative would be to always take-by-value, allowing it to be worked around by calling .clone before passing:

let mut buf = ...;
let my_t = ...;
buf.fill(my_t.clone());
// my_t is still available here

However this also seems somewhat awkward given that the type already implements Clone yet we're still forced to do a manual clone at the entry point. It's also strictly slower if the Clone is backed by an Arc we're forcing an extra atomic operation.

@lcnr
Copy link
Contributor

lcnr commented Apr 15, 2020

Using T is also fine with me, this would be similar to iter::repeat, which also always stores a T which cannot be used. link

However this also seems somewhat awkward given that the type already implements Clone yet we're still forced to do a manual clone at the entry point. It's also strictly slower since if the Clone is backed by an Arc we're forcing an extra atomic operation.

When filling a slice of Arc<T>? Considering that we repeatedly clone the arc inside of fill I doubt that 1 additional clone will ever cause a problem in this case. We could use the following, which ends up generating the same asm for copy types and saves 1 clone.

if let Some((start, rest)) = slice.split_first_mut() {
    for el in rest {
        el.clone_from(&value)
    }

    *start = value;
}

https://rust.godbolt.org/z/nXzWMM

@dtolnay
Copy link
Member

dtolnay commented Apr 15, 2020

This does make the common case somewhat awkward:

let mut buf = vec![0; 10];
buf.fill(1);  // compiler error!
buf.fill(&1); // ok

We should consider that this is exactly what "discarding ownership" is intended to address. Rather than being super generic as Simon pointed out this can damage type inference, there's nothing wrong with allowing buf.fill(1) at the call site even with the non-generic &T argument type. In a "discarding ownership" world a non-generic &T argument seems like it would be a better choice here.

@cuviper
Copy link
Member

cuviper commented Apr 15, 2020

This does make the common case somewhat awkward:

let mut buf = vec![0; 10];
buf.fill(1);  // compiler error!
buf.fill(&1); // ok
assert_eq!(buf, vec![1; 10]);

FWIW, this is true of slice contains too, requiring examples like v.contains(&30).

@SimonSapin
Copy link
Contributor

Consistency with contains plus the possibility of that borrowing becoming implicit in the future with discarding ownership sounds convincing to me for having fill take &T.

@ollie27
Copy link
Member

ollie27 commented Apr 15, 2020

vec![x; n] takes T by value so I would argue for consistency fill should as well. The main advantage is that it only needs n - 1 calls to clone().

@SimonSapin
Copy link
Contributor

The motivation for having fill in the first place is the cases where it optimizes to memset, so I feel that saving one .clone() is not a priority.

@ollie27
Copy link
Member

ollie27 commented Apr 16, 2020

What would be the advantage to taking &T as opposed to T by-value?

@cuviper
Copy link
Member

cuviper commented Apr 16, 2020

&T lets you borrow an existing value without manually cloning it, including the slight advantage of creating no clones at all for the degenerate case of an empty slice.

@ollie27
Copy link
Member

ollie27 commented Apr 16, 2020

&T lets you borrow an existing value without manually cloning it

Without something like "discarding ownership" you would have to manually reference it instead.

slight advantage of creating no clones at all for the degenerate case of an empty slice

With by-value we could avoid an extra clone in the likely common case where you no longer need the value and the slice isn't empty.

As far as consistency is concerned Vec::resize also takes T by-value.

@cuviper
Copy link
Member

cuviper commented Apr 16, 2020

FWIW, I'm not firm on it being &T -- it seems pretty reasonable to argue that fill is more similar to Vec::resize than slice::contains.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 3, 2020
`slice::fill`: use `T` instead of generic arg

implements rust-lang#70758 (comment)

As the discussion in rust-lang#70758 has shifted, I now use `T` instead of `&T`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 3, 2020
`slice::fill`: use `T` instead of generic arg

implements rust-lang#70758 (comment)

As the discussion in rust-lang#70758 has shifted, I now use `T` instead of `&T`.
@KodrAus KodrAus added A-slice Area: `[T]` Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 13, 2020

Since fill is similar to Vec::resize, should there also be a fill_with, just like resize_with?

@yoshuawuyts
Copy link
Member Author

@m-ou-se I think that's a reasonable addition; though it could be useful to track separately so we can stabilize the two functions independently?

@m-ou-se
Copy link
Member

m-ou-se commented Nov 19, 2020

Sure. Just mentioning it here because fill_with could resolve some of the concerns with fill. If fill takes a T by value, fill_with could be a solution for the few cases where that might be undesirable. And it'd make it consistent with Vec::resize, which is a good argument for making fill follow the same signature and take T by value.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 19, 2020

I think slice::fill is ready for a stabilization PR. There's been no discussion about the signature since May, when it was changed to take a T.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Nov 19, 2020

@m-ou-se I agree; filed a stabilization PR in #79213

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 21, 2020
Add `core::slice::fill_with`

Tracking issue rust-lang#79221.

As suggested by `@m-ou-se` in rust-lang#70758 (comment) this implements `slice::fill_with` as a counterpart to `slice::fill`. This mirrors `Vec::resize` and `Vec::resize_with`. Thanks!

r? `@m-ou-se`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 21, 2020
Add `core::slice::fill_with`

Tracking issue rust-lang#79221.

As suggested by `@m-ou-se` in rust-lang#70758 (comment) this implements `slice::fill_with` as a counterpart to `slice::fill`. This mirrors `Vec::resize` and `Vec::resize_with`. Thanks!

r? `@m-ou-se`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 20, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? `@m-ou-se`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 20, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? ``@m-ou-se``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 25, 2020
…=m-ou-se

Stabilize `core::slice::fill`

Tracking issue rust-lang#70758

Stabilizes the `core::slice::fill` API in Rust 1.50, adding a `memset` doc alias so people coming from C/C++ looking for this operation can find it in the docs. This API hasn't seen any changes since we changed the signature in rust-lang#71165, and it seems like the right time to propose stabilization. Thanks!

r? `@m-ou-se`
@yoshuawuyts
Copy link
Member Author

Closing as the PR has been merged and stabilized for Rust 1.50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants