Add Vec::resize_default. #41771

clarfonthey · 2017-05-05T18:43:37Z

As suggested by #41758.

rust-highfive · 2017-05-05T18:43:48Z

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2017-05-05T19:10:09Z

src/libcollections/vec.rs

+    ///
+    /// 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.


Maybe add a note "See also Vec::resize, which uses clone."? I don't know what policy is on that, though (cc @rust-lang/docs)

I will fix the docs later today

Linking to our methods is good, we do that in other places. ~~What does 'cloning' have to do with this method?~~ Seems like it should say something along the lines of: "If you'd like to specify your own value if the Vec is extended, use Vec::resize".

I've updated the docs to reference both methods in each other.

nikomatsakis · 2017-05-05T19:12:28Z

src/libcollections/vec.rs

+    /// assert_eq!(vec, [1, 2]);
+    /// ```
+    #[unstable(feature = "vec_resize_default", issue = "41758")]
+    pub fn resize_default(&mut self, new_len: usize, value: T) {


surely it's not supposed to take a value argument?

this is what I get for making a PR right before running to catch the bus

(I fixed this btw)

bluss · 2017-05-06T12:19:20Z

src/libcollections/vec.rs

@@ -1301,6 +1301,41 @@ impl<T: Clone> Vec<T> {
    }
 }

+impl<T: Default> Vec<T> {


Just a thought, separate impl blocks for different bounds seems archaic (from the time before where clauses), so we probably don't want to continue that.

Hmm. I agree, although for now I think it'd be best to continue the pattern and refactor the entire file in a future PR.

Also, this seems like a good candidate for a fmt RFC.

frewsxcv · 2017-05-07T14:29:28Z

src/libcollections/vec.rs

@@ -1301,6 +1306,47 @@ impl<T: Clone> Vec<T> {
    }
 }

+impl<T: Default> Vec<T> {
+    /// Resizes the `Vec` in-place so that `len()` is equal to `new_len`.


nit: we (the docs team) for the time being, don't add () to the end of method names in documentation, so this len() should be len, ideally linking to the associated method. alternatively, you could just say "...so that its length is equal to new_len." not a big deal though

Will modify on both methods! This was copy-pasted from the old docs.

carols10cents · 2017-05-08T14:31:16Z

Looks like there's a test failure:

[01:03:01] failures:
[01:03:01] 
[01:03:01] ---- vec.rs - vec::Vec<T>::resize_default (line 1321) stdout ----
[01:03:01] 	error: use of unstable library feature 'vec_resize_default' (see issue #41758)
[01:03:01]  --> <anon>:5:5
[01:03:01]   |
[01:03:01] 5 | vec.resize_default(5);
[01:03:01]   |     ^^^^^^^^^^^^^^
[01:03:01]   |
[01:03:01]   = help: add #![feature(vec_resize_default)] to the crate attributes to enable
[01:03:01] 
[01:03:01] error: use of unstable library feature 'vec_resize_default' (see issue #41758)
[01:03:01]  --> <anon>:9:5
[01:03:01]   |
[01:03:01] 9 | vec.resize_default(2);
[01:03:01]   |     ^^^^^^^^^^^^^^
[01:03:01]   |
[01:03:01]   = help: add #![feature(vec_resize_default)] to the crate attributes to enable
[01:03:01] 
[01:03:01] error: aborting due to previous error(s)
[01:03:01] 
[01:03:01] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc/session/mod.rs:216
[01:03:01] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:03:01] 
[01:03:01] 
[01:03:01] failures:
[01:03:01]     vec.rs - vec::Vec<T>::resize_default (line 1321)
[01:03:01] 
[01:03:01] test result: FAILED. 404 passed; 1 failed; 4 ignored; 0 measured
[01:03:01] 
[01:03:01] error: test failed, to rerun pass '--doc'
[01:03:01]

nikomatsakis · 2017-05-08T18:03:09Z

src/libcollections/vec.rs

+    ///
+    /// [`resize`]: #method.resize
+    #[unstable(feature = "vec_resize_default", issue = "41758")]
+    pub fn resize_default(&mut self, new_len: usize) {


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?).

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.

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.

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.

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

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.

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

Mark-Simulacrum · 2017-05-10T21:38:57Z

A few warnings to remove, and then probably ready for re-review.

[00:01:40] error: unused variable: `count`
[00:01:40]     --> /checkout/src/libcollections/vec.rs:1337:17
[00:01:40]      |
[00:01:40] 1337 |             let count = new_len - len;
[00:01:40]      |                 ^^^^^
[00:01:40]      |
[00:01:40]      = note: #[deny(unused_variables)] implied by #[deny(warnings)]
[00:01:40] note: lint level defined here
[00:01:40]     --> /checkout/src/libcollections/lib.rs:30:9
[00:01:40]      |
[00:01:40] 30   | #![deny(warnings)]
[00:01:40]      |         ^^^^^^^^
[00:01:40] 
[00:01:40] error: aborting due to previous error

clarfonthey · 2017-05-11T02:18:49Z

So, I hopefully fixed the errors and also generalised extend_with_element and extend_with_default with a trait that's admittedly kind of ugly, but small enough and gets the job done.

Mark-Simulacrum · 2017-05-11T02:41:10Z

Travis is failing. @clarcharr Just to check, are you able to build locally? Is there something we can help resolve to assist with that? (It's fine if you use travis for tests, just want to make sure that there's nothing on our side blocking your ability to build locally).

[00:01:38] error[E0277]: the trait bound `T: core::clone::Clone` is not satisfied
[00:01:38]     --> /checkout/src/libcollections/vec.rs:1324:9
[00:01:38]      |
[00:01:38] 1324 | impl<T> ExtendWith<T> for ExtendElement<T> {
[00:01:38]      |         ^^^^^^^^^^^^^ the trait `core::clone::Clone` is not implemented for `T`
[00:01:38]      |
[00:01:38]      = help: consider adding a `where T: core::clone::Clone` bound
[00:01:38]      = note: required by `vec::ExtendElement`
[00:01:38] 
[00:01:38] error[E0277]: the trait bound `T: core::clone::Clone` is not satisfied
[00:01:38]     --> /checkout/src/libcollections/vec.rs:1325:5
[00:01:38]      |
[00:01:38] 1325 |     fn next(&self) -> T { self.0.clone() }
[00:01:38]      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `core::clone::Clone` is not implemented for `T`
[00:01:38]      |
[00:01:38]      = help: consider adding a `where T: core::clone::Clone` bound
[00:01:38]      = note: required by `vec::ExtendElement`
[00:01:38] 
[00:01:38] error[E0277]: the trait bound `T: core::clone::Clone` is not satisfied
[00:01:38]     --> /checkout/src/libcollections/vec.rs:1326:5
[00:01:38]      |
[00:01:38] 1326 |     fn last(self) -> T { self.0 }
[00:01:38]      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `core::clone::Clone` is not implemented for `T`
[00:01:38]      |
[00:01:38]      = help: consider adding a `where T: core::clone::Clone` bound
[00:01:38]      = note: required by `vec::ExtendElement`
[00:01:38] 
[00:01:38] error: aborting due to 3 previous errors

nikomatsakis · 2017-05-11T13:42:22Z

src/libcollections/vec.rs

+// This code generalises `extend_with_{element,default}`.
+trait ExtendWith<T> {
+    fn next(&self) -> T;
+    fn last(self) -> T { self.next() }


Nit: Given that this is a private trait with two impls, I'd say remove the default impl and just move this method to the ExtendDefault trait.

But this is clever.

scottmcm · 2017-05-11T18:22:22Z

src/libcollections/vec.rs

    /// 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.


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.

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.

Ah, I was unaware. Sorry for the noise.

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

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.

clarfonthey · 2017-05-11T18:37:12Z

@Mark-Simulacrum There isn't actually anything preventing me from testing locally, other than the fact that I need to remember to set my computer down for an hour and build once so that I can actually test quickly. I've been moving around a lot the past few days and haven't had time. :P

clarfonthey · 2017-05-12T19:25:23Z

Note: tests pass now. Is there anything left to do before merge? @nikomatsakis

nikomatsakis · 2017-05-15T19:12:54Z

@bors r+

bors · 2017-05-15T19:12:55Z

📌 Commit c2c0641 has been approved by nikomatsakis

bors · 2017-05-15T21:52:21Z

⌛ Testing commit c2c0641 with merge b89bcf5...

bors · 2017-05-15T21:55:12Z

💔 Test failed - status-appveyor

alexcrichton · 2017-05-15T22:11:39Z

@bors: retry

network issues Tracking issue for spurious network failures on bots #40474

…sakis Add Vec::resize_default. As suggested by rust-lang#41758.

bors · 2017-05-16T08:14:35Z

⌛ Testing commit c2c0641 with merge 4d09a0e...

Add Vec::resize_default. As suggested by #41758.

bors · 2017-05-16T10:59:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4d09a0e to master...

rust-highfive assigned nikomatsakis May 5, 2017

Mark-Simulacrum reviewed May 5, 2017

View reviewed changes

nikomatsakis requested changes May 5, 2017

View reviewed changes

bluss reviewed May 6, 2017

View reviewed changes

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2017

frewsxcv reviewed May 7, 2017

View reviewed changes

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2017

nikomatsakis reviewed May 8, 2017

View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 9, 2017

clarfonthey force-pushed the resize_default branch from 6daa187 to a03ff4e Compare May 9, 2017 20:54

nikomatsakis approved these changes May 10, 2017

View reviewed changes

clarfonthey force-pushed the resize_default branch from a03ff4e to 951647e Compare May 11, 2017 02:13

nikomatsakis reviewed May 11, 2017

View reviewed changes

Add Vec::resize_default.

c2c0641

clarfonthey force-pushed the resize_default branch from 951647e to c2c0641 Compare May 11, 2017 16:56

scottmcm reviewed May 11, 2017

View reviewed changes

nikomatsakis approved these changes May 15, 2017

View reviewed changes

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 16, 2017

Rollup merge of rust-lang#41771 - clarcharr:resize_default, r=nikomat…

403fde8

…sakis Add Vec::resize_default. As suggested by rust-lang#41758.

frewsxcv mentioned this pull request May 16, 2017

Rollup of 6 pull requests #42022

Closed

bors added a commit that referenced this pull request May 16, 2017

Auto merge of #41771 - clarcharr:resize_default, r=nikomatsakis

4d09a0e

Add Vec::resize_default. As suggested by #41758.

bors merged commit c2c0641 into rust-lang:master May 16, 2017

clarfonthey deleted the resize_default branch May 20, 2017 19:05

clarfonthey mentioned this pull request May 16, 2017

Warn methods in doc comments written as fun() instead of fun rust-lang/rust-clippy#1750

Open

Add Vec::resize_default. #41771

Add Vec::resize_default. #41771

Conversation

clarfonthey commented May 5, 2017 • edited Loading

rust-highfive commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented May 8, 2017

nikomatsakis May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented May 10, 2017

clarfonthey commented May 11, 2017

Mark-Simulacrum commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented May 11, 2017

clarfonthey commented May 12, 2017

nikomatsakis commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

alexcrichton commented May 15, 2017

bors commented May 16, 2017

bors commented May 16, 2017

clarfonthey commented May 5, 2017 •

edited

Loading

frewsxcv May 6, 2017 •

edited

Loading

nikomatsakis May 8, 2017 •

edited

Loading

clarfonthey May 11, 2017 •

edited

Loading