Tracking Issue for the slice::fill method

[yoshuawuyts]

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

• Adjust documentation (see instructions on rustc-dev-guide)
• Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

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

Implementation history

• Initial implementation in Add slice::fill #70752
• Changed the signature in slice::fill: use T instead of generic arg #71165
• Stabilization PR in Stabilize core::slice::fill #79213

[cuviper]

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]

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]

+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]

+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]

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]

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]

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]

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]

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]

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]

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

[cuviper]

&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]

&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]

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.

[m-ou-se]

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

[yoshuawuyts]

@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]

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]

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]

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

[yoshuawuyts]

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