Add back Vec::from_elem

[Gankra]

rendered

[Valloric]

+1, we need from_elem in the Vec API for all the reasons stated in the RFC.

[alexcrichton]

Another drawback you may wish to add is inconsistency in APIs. This doesn't also include other list-like collections such as DList, Bitv, or RingBuf.

[mahkoh]

// #4
let vec: Vec<_> = (0..n).map(|_| elem.clone()).collect()
// #5
let vec: Vec<_> = iter::repeat(elem).take(n).collect();

#4 and #5 suffer from the untrusted iterator len problem performance wise (which is only a temporary argument, this will be solved sooner rather than later), and are otherwise verbose and noisy. They also need to clone one more time than other methods strictly need to.

Given that the performance argument is only temporary, the only argument here seems to be the verbosity. I don't buy this argument because push_all is still scheduled for removal and the only way to extend a Vec from a &[T] where T is clone seems to be

v.extend(x.iter().map(|b|b.clone()));

which is much more verbose than

v.push_all(x);

[alexcrichton]

To me personally the 5th option is indeed the idiomatic method by which I would recommend creating a Vec today. Opinions of verboseness/noisiness aside, it would be useful to quantify precisely why this this is slow beyond the trusted iterator length problem, as well as backing this claim up with numbers.

I've created a sample benchmark which today resolves the performance argument for options 4/5 (at least for this one use case). This is boiling down to me trying to say that performance is a fixable problem with the exact APIs we have today (especially with trusted iterator lengths) and it may not be a strong enough motivation for this change.

[Gankra]

I was basing these claims based on these results I found a month ago.

[alexcrichton]

Thanks for the link! They're quite helpful.

I removed the from_elem benchmarks (as it was removed) and got the following results with today's compiler plus the modified version from above: https://gist.github.com/alexcrichton/1b17e3ace982c095a49d. The results are pretty surprising to me and may indicate that an LLVM update has had a regression recently?

[Gankra]

My laptop was acting super inconsistent when I ran the benches, so I wouldn't be surprised if my results were erroneous. Although iirc Unsafe consistently beat FromIter...

I'd also warn that your trusted_len isn't panic safe (not clear if trusted_len should imply no_panic). disregard. See rust-lang/rust#21465 for detailed discussion.

[Gankra]

Updated my old bench to master, stealing Vec::from_elem from 0.11: https://gist.github.com/Gankro/f40d87b75ae91562892f

Same basic results you got: my trusted impl is busted!

[tbu-]

That isn't totally sound: Why is the unsafe path slower than the safe path?

[aturon]

@mahkoh

v.extend(x.iter().cloned())

[brson]

SGTM.

Did not know that vec![n; m] existed.

[pythonesque]

Yes, please. How to do this is one of the most common questions from newcomers currently.

[Gankra]

Here's an implementation of a tricked out vec! macro:

macro_rules! vec2 {
    ($x:expr; $y:expr) => (
        unsafe {
            use std::ptr;
            use std::clone::Clone;

            let elem = $x;
            let n: usize = $y;
            let mut v = Vec::with_capacity(n);
            let mut ptr = v.as_mut_ptr();
            for i in range(1, n) {
                ptr::write(ptr, Clone::clone(&elem));
                ptr = ptr.offset(1);
                v.set_len(i);
            }

            // No needless clones
            if n > 0 {
                ptr::write(ptr, elem);
                v.set_len(n);
            }

            v
        }
    );
    ($($x:expr),*) => (
        <[_] as std::slice::SliceExt>::into_vec(
            std::boxed::Box::new([$($x),*]))
    );
    ($($x:expr,)*) => (vec![$($x),*])
}

fn main() {
    println!("{:?}", vec2![1; 10]);
    println!("{:?}", vec2![Box::new(1); 10]);
    let n = 10;
    println!("{:?}", vec2![1; n]);
}

[Gankra]

(this can of course be abstracted to a function to avoid code bloat issues)

[rkjnsn]

+1 to making vec![elem; n] more flexible.

[aturon]

@gankro can you switch the RFC to prefer a generalized vec![e, n] design, which seems to be the consensus (and has basically no drawbacks afaict).

[Gankra]

@aturon there's one drawback: macros don't have privacy. So either you're always inlining the code, or you're pasting the function definition everywhere. Apparently we can (and do) coalesce all the identical definitions, but it's not the best thing to do. This is of course largely an implementation detail, and I'm told that macros 2.0 will have some way for macros to have private scopes they can draw from.

[aturon]

@gankro yep, that comes up with several of the current macros. We have various ways of dealing with this (hiding the docs, really silly names, etc). I likewise consider it an implementation detail that can/will be improved over time.

[Gankra]

vec![x; n] upgrade is now the primary proposal.

[alexcrichton]

We currently unfortunately lack unsafe hygiene, allowing unsafe expressions in $x and $y without an explicitly declared unsafe block. Additionally, I think that this would trigger an "unused unsafe" warning for unsafe { vec![a; b] } due to the macro-generated unsafe block not being necessary.

Perhaps a definition like this could be used to solve both these problems?

macro_rules! vec {
    ($e:expr; $n:expr) =>> {
        fn __vec_from_elem<T: ::std::clone::Clone>(t: T, n: usize) -> Vec<T> {
            // all other impl details here, including an `unsafe` block
        }
        __vec_from_elem($e, $n)
    }
}

[Gankra]

Oh dang, good point. I was basically expecting to have to split out a fn anyway for inline-countering reasons. Written as-is for simplicity.

[alexcrichton]

@gankro this sounds like a fantastic "middle ground", thanks for drawing this up!

[aturon]

This RFC has been merged after an overwhelmingly positive response. Thanks @gankro!

Tracking issue