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

Add Vec::resize_default. #41771

Merged
merged 1 commit into from
May 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/doc/unstable-book/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@
- [unique](library-features/unique.md)
- [unsize](library-features/unsize.md)
- [utf8_error_error_len](library-features/utf8-error-error-len.md)
- [vec_resize_default](library-features/vec-resize-default.md)
- [vec_remove_item](library-features/vec-remove-item.md)
- [windows_c](library-features/windows-c.md)
- [windows_handle](library-features/windows-handle.md)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# `vec_resize_default`

The tracking issue for this feature is: [#41758]

[#41758]: https://github.com/rust-lang/rust/issues/41758

------------------------
128 changes: 95 additions & 33 deletions src/libcollections/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1220,11 +1220,14 @@ impl<T> Vec<T> {
}

impl<T: Clone> Vec<T> {
/// Resizes the `Vec` in-place so that `len()` is equal to `new_len`.
/// Resizes the `Vec` in-place so that `len` is equal to `new_len`.
///
/// If `new_len` is greater than `len()`, the `Vec` is extended by the
/// If `new_len` is greater than `len`, the `Vec` is extended by the
/// difference, with each additional slot filled with `value`.
/// If `new_len` is less than `len()`, the `Vec` is simply truncated.
/// If `new_len` is less than `len`, the `Vec` is simply truncated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since len is a method, not a variable, I think it makes sense for it to be new_len (variable) and len() (method) in the comment, as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based upon the earlier suggestion by @frewsxcv; the docs team has decided to not put the parentheses on the methods, so, I will go with the convention says.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was unaware. Sorry for the noise.

Copy link
Contributor Author

@clarfonthey clarfonthey May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I filed Manishearth/rust-clippy#1750 so that we can maybe lint for this convention.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I find that new convention pretty confusing! I agree that this PR ought to follow it, but I think we ought to consider changing it.

///
/// This method requires `Clone` to clone the passed value. If you'd
/// rather create a value with `Default` instead, see [`resize_default`].
///
/// # Examples
///
Expand All @@ -1237,19 +1240,100 @@ impl<T: Clone> Vec<T> {
/// vec.resize(2, 0);
/// assert_eq!(vec, [1, 2]);
/// ```
///
/// [`resize_default`]: #method.resize_default
#[stable(feature = "vec_resize", since = "1.5.0")]
pub fn resize(&mut self, new_len: usize, value: T) {
let len = self.len();

if new_len > len {
self.extend_with_element(new_len - len, value);
self.extend_with(new_len - len, ExtendElement(value))
} else {
self.truncate(new_len);
}
}

/// Clones and appends all elements in a slice to the `Vec`.
///
/// Iterates over the slice `other`, clones each element, and then appends
/// it to this `Vec`. The `other` vector is traversed in-order.
///
/// Note that this function is same as `extend` except that it is
/// specialized to work with slices instead. If and when Rust gets
/// specialization this function will likely be deprecated (but still
/// available).
///
/// # Examples
///
/// ```
/// let mut vec = vec![1];
/// vec.extend_from_slice(&[2, 3, 4]);
/// assert_eq!(vec, [1, 2, 3, 4]);
/// ```
#[stable(feature = "vec_extend_from_slice", since = "1.6.0")]
pub fn extend_from_slice(&mut self, other: &[T]) {
self.spec_extend(other.iter())
}
}

impl<T: Default> Vec<T> {
/// Resizes the `Vec` in-place so that `len` is equal to `new_len`.
///
/// If `new_len` is greater than `len`, the `Vec` is extended by the
/// difference, with each additional slot filled with `Default::default()`.
/// If `new_len` is less than `len`, the `Vec` is simply truncated.
///
/// This method uses `Default` to create new values on every push. If
/// you'd rather `Clone` a given value, use [`resize`].
///
///
/// # Examples
///
/// ```
/// #![feature(vec_resize_default)]
///
/// let mut vec = vec![1, 2, 3];
/// vec.resize_default(5);
/// assert_eq!(vec, [1, 2, 3, 0, 0]);
///
/// let mut vec = vec![1, 2, 3, 4];
/// vec.resize_default(2);
/// assert_eq!(vec, [1, 2]);
/// ```
///
/// [`resize`]: #method.resize
#[unstable(feature = "vec_resize_default", issue = "41758")]
pub fn resize_default(&mut self, new_len: usize) {
Copy link
Contributor

@nikomatsakis nikomatsakis May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, should we just implement this with self.resize(new_len, T::default())? I guess it would invoke default even when you don't need it...but it seems odd for this implementation to stray from the other resize (e.g., that version calls self.extend_with_element(), maybe we should do the same?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but I decided we couldn't since that would require T: Clone, right? Same for calling self.extend_with_element(). Ideally we'd have some trait that is "produce more of the same value" which would be be implemented for both T: Default and T: Clone. Unless we have an impl impl Clone for T where T: Default, which could work (i.e., calling Default::default each time, but might be impossible to add today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was mentioning about a more efficient implementation is that we could specialise this to just call extend_with_element for Clone + Default.

I can add this before merge if it's desired for the initial PR, although I might not be free this week to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also potentially add a clippy lint that warns if a type implements Default but not Clone, but IDK if that's too niche.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's true, I forgot that Default doesn't imply Clone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, ok, I give up =) I was going to suggest maybe combining extend_with_{element,default} into one thing that takes a closure, but that seems to require an extra clone. Seems ok as is I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and figure something out; I have a feeling that I know what to do.

let len = self.len();

if new_len > len {
self.extend_with(new_len - len, ExtendDefault);
} else {
self.truncate(new_len);
}
}
}

/// Extend the vector by `n` additional clones of `value`.
fn extend_with_element(&mut self, n: usize, value: T) {
// This code generalises `extend_with_{element,default}`.
trait ExtendWith<T> {
fn next(&self) -> T;
fn last(self) -> T;
}

struct ExtendElement<T>(T);
impl<T: Clone> ExtendWith<T> for ExtendElement<T> {
fn next(&self) -> T { self.0.clone() }
fn last(self) -> T { self.0 }
}

struct ExtendDefault;
impl<T: Default> ExtendWith<T> for ExtendDefault {
fn next(&self) -> T { Default::default() }
fn last(self) -> T { Default::default() }
}
impl<T> Vec<T> {
/// Extend the vector by `n` values, using the given generator.
fn extend_with<E: ExtendWith<T>>(&mut self, n: usize, value: E) {
self.reserve(n);

unsafe {
Expand All @@ -1261,43 +1345,21 @@ impl<T: Clone> Vec<T> {

// Write all elements except the last one
for _ in 1..n {
ptr::write(ptr, value.clone());
ptr::write(ptr, value.next());
ptr = ptr.offset(1);
// Increment the length in every step in case clone() panics
// Increment the length in every step in case next() panics
local_len.increment_len(1);
}

if n > 0 {
// We can write the last element directly without cloning needlessly
ptr::write(ptr, value);
ptr::write(ptr, value.last());
local_len.increment_len(1);
}

// len set by scope guard
}
}

/// Clones and appends all elements in a slice to the `Vec`.
///
/// Iterates over the slice `other`, clones each element, and then appends
/// it to this `Vec`. The `other` vector is traversed in-order.
///
/// Note that this function is same as `extend` except that it is
/// specialized to work with slices instead. If and when Rust gets
/// specialization this function will likely be deprecated (but still
/// available).
///
/// # Examples
///
/// ```
/// let mut vec = vec![1];
/// vec.extend_from_slice(&[2, 3, 4]);
/// assert_eq!(vec, [1, 2, 3, 4]);
/// ```
#[stable(feature = "vec_extend_from_slice", since = "1.6.0")]
pub fn extend_from_slice(&mut self, other: &[T]) {
self.spec_extend(other.iter())
}
}

// Set the length of the vec when the `SetLenOnDrop` value goes out of scope.
Expand Down Expand Up @@ -1389,7 +1451,7 @@ trait SpecFromElem: Sized {
impl<T: Clone> SpecFromElem for T {
default fn from_elem(elem: Self, n: usize) -> Vec<Self> {
let mut v = Vec::with_capacity(n);
v.extend_with_element(n, elem);
v.extend_with(n, ExtendElement(elem));
v
}
}
Expand Down Expand Up @@ -1424,7 +1486,7 @@ macro_rules! impl_spec_from_elem {
}
}
let mut v = Vec::with_capacity(n);
v.extend_with_element(n, elem);
v.extend_with(n, ExtendElement(elem));
v
}
}
Expand Down