Introduce Vec::resize_with method (see #41758) #49559
Conversation
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/liballoc/vec.rs
Outdated
| /// let mut vec = vec![1, 2, 3, 4]; | ||
| /// vec.resize_with(2, Default::default); | ||
| /// assert_eq!(vec, [1, 2]); | ||
| /// ``` |
There was a problem hiding this comment.
Since it's FnMut, consider adding an example that uses state of some kind. Perhaps
let mut vec = vec![];
let mut p = 1;
vec.resize_with(4, || { p *= 2; p });
assert_eq!(vec, [2, 4, 8, 16]);There was a problem hiding this comment.
Yup, that was on my list but forgot about it. I've updated it now, using your example.
src/liballoc/vec.rs
Outdated
| pub fn resize_with<F>(&mut self, new_len: usize, mut f: F) | ||
| where F: FnMut() -> T | ||
| { | ||
| let len = self.len(); |
There was a problem hiding this comment.
Should the be implemented using extend_with, like the other two are? Something like
struct ExtendFunc<F>(F);
impl<T, F: FnMut() -> T> ExtendWith<T> for ExtendFunct<F> {
fn next(&self) -> T { (self.0)() }
fn last(self) -> T { (self.0)() }
}(Would it ever be possible for a closure to implement its FnOnce differently from its FnMut, to take advantage of the difference here the way ExtendElement does?)
There was a problem hiding this comment.
As I understand it, the value returned by f is already moved into place for each iteration in the current implementation of resize_with(), so it seems like the ExtendWith mechanism doesn't make sense here.
There was a problem hiding this comment.
I believe this should use ExtendWith: ExtendWith is not only used to generalize and avoid the last clone, extend_with is also implemented in a way that makes optimizations more likely, as I understand the code.
There was a problem hiding this comment.
Hmm, I think that runs into mutability problems. ExtendWith::next() is defined as taking &self, in which case the mutability of the FnMut seems inaccessible through implementing ExtendFunc the way @scottmcm proposes. I don't think I see a way out of that...
There was a problem hiding this comment.
Good point! ExtendWith is non-pub, though, so maybe just change it to take &mut self? It doesn't look like an instance of it ever needs to be shared...
src/liballoc/vec.rs
Outdated
| /// | ||
| /// If `new_len` is greater than `len`, the `Vec` is extended by the | ||
| /// difference, with each additional slot filled with the result of | ||
| /// closure `f`. If `new_len` is less than `len`, the `Vec` is simply |
There was a problem hiding this comment.
nit: I'd make this "[...] with the result of calling the closure f".
| @@ -1306,6 +1306,49 @@ impl<T> Vec<T> { | |||
| } | |||
| other | |||
| } | |||
|
|
|||
| /// Resizes the `Vec` in-place so that `len` is equal to `new_len`. | |||
There was a problem hiding this comment.
I'd mention in the docs the order in which the elements are inserted / f is called, since that is something people would want to rely on, I imagine.
There was a problem hiding this comment.
I added "The return values from f will end up in the Vec in the order they have been generated." The wording here feels a little awkward to me, maybe someone (a native speaker) can tighten it up?
There was a problem hiding this comment.
The "end up in" is maybe a bit colloquial, but it's easily understandable, so I'm happy.
src/liballoc/vec.rs
Outdated
| @@ -1333,7 +1376,8 @@ impl<T: Clone> Vec<T> { | |||
| /// | |||
| /// [`Clone`]: ../../std/clone/trait.Clone.html | |||
| /// [`Default`]: ../../std/default/trait.Default.html | |||
| /// [`resize_default`]: #method.resize_default | |||
| /// ['Default::default`]: ../../std/default/trait.Default.html#method.default | |||
There was a problem hiding this comment.
I don't believe this link is used.
src/liballoc/vec.rs
Outdated
| pub fn resize_with<F>(&mut self, new_len: usize, mut f: F) | ||
| where F: FnMut() -> T | ||
| { | ||
| let len = self.len(); |
There was a problem hiding this comment.
I believe this should use ExtendWith: ExtendWith is not only used to generalize and avoid the last clone, extend_with is also implemented in a way that makes optimizations more likely, as I understand the code.
|
Oh since this is unstable it's fine to not need FCP to land this |
|
@TimNN @alexcrichton any suggestions on how to handle |
|
@djc could |
pietroalbini
added
the
S-waiting-on-review
|
@alexcrichton (or other Libs team members) it probably could. My question is more: given the discussion in the tracking issue, supposedly |
|
@djc oh we'd want the replacement to be stable before taking any action, so for this I think it's fine to just add the new method unstable and leave |
|
@bors r+ |
|
📌 Commit da0ceef has been approved by |
bors
added
S-waiting-on-bors
Introduce Vec::resize_with method (see rust-lang#41758) In rust-lang#41758, the libs team decided they preferred `Vec::resize_with` over `Vec::resize_default()`. Here is an implementation to get this moving forward. I don't know what the removal process for `Vec::resize_default()` should be, so I've left it in place for now. Would be happy to follow up with its removal.
Rollup of 25 pull requests Successful merges: - #49179 (Handle future deprecation annotations ) - #49512 (Add support for variant and types fields for intra links) - #49515 (fix targetted value background) - #49516 (Add missing anchor for union type fields) - #49532 (Add test for rustdoc ignore test) - #49533 (Add #[must_use] to a few standard library methods) - #49540 (Fix miri Discriminant() for non-ADT) - #49559 (Introduce Vec::resize_with method (see #41758)) - #49570 (avoid IdxSets containing garbage above the universe length) - #49577 (Stabilize String::replace_range) - #49599 (Fix typo) - #49603 (Fix url for intra link provided method) - #49607 (Stabilize iterator methods in 1.27) - #49609 (run-pass/attr-stmt-expr: expand test cases) - #49612 (Fix "since" version for getpid feature.) - #49618 (Fix build error when compiling libcore for 16bit targets) - #49619 (tweak core::fmt docs) - #49637 (Stabilize parent_id()) - #49639 (Update Cargo) - #49628 (Re-write the documentation index) - #49594 (Add some performance guidance to std::fs and std::io docs) - #49625 (miri: add public alloc_kind accessor) - #49634 (Add a test for the fix to issue #43058) - #49641 (Regression test for #46314) - #49547 (Unignore borrowck test) Failed merges:
In #41758, the libs team decided they preferred
Vec::resize_withoverVec::resize_default(). Here is an implementation to get this moving forward.I don't know what the removal process for
Vec::resize_default()should be, so I've left it in place for now. Would be happy to follow up with its removal.