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

Tracking issue for Vec::resize_default #41758

Open
joshlf opened this Issue May 4, 2017 · 15 comments

Comments

Projects
None yet
7 participants
@joshlf
Contributor

joshlf commented May 4, 2017

Currently, Vec has a resize method which takes a new parameter so that if the resize involves growing the vector, the new parameter is cloned into each newly-created cell in the vector. T must implement Clone in order for this method to be available.

It would be useful to add a resize_default method that instead requires that T: Default, and calls T::default() to fill the newly-created cells. Not only would this be ergonomic, but for certain implementations of T::default, it might allow the compiler to make better optimizations.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 5, 2017

Contributor

This would be an interesting addition for types which support Default but not Clone. I'll try and make a PR for it.

In the future, just FYI, feature requests should go in the RFCs repository.

Contributor

clarcharr commented May 5, 2017

This would be an interesting addition for types which support Default but not Clone. I'll try and make a PR for it.

In the future, just FYI, feature requests should go in the RFCs repository.

@joshlf

This comment has been minimized.

Show comment
Hide comment
@joshlf

joshlf May 5, 2017

Contributor

Ah OK, noted. That's even for feature requests like this one that don't deserve an RFC?

Contributor

joshlf commented May 5, 2017

Ah OK, noted. That's even for feature requests like this one that don't deserve an RFC?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 15, 2017

Contributor

Yep! The general rule is either PR here or RFC elsewhere.

Also, @joshlf would you mind renaming this issue to "Tracking issue for Vec::resize_default" ?

And @nikomatsakis would you mind tagging it?

Contributor

clarcharr commented May 15, 2017

Yep! The general rule is either PR here or RFC elsewhere.

Also, @joshlf would you mind renaming this issue to "Tracking issue for Vec::resize_default" ?

And @nikomatsakis would you mind tagging it?

@joshlf joshlf changed the title from Add Vec::resize_default method to Tracking issue for Vec::resize_default May 15, 2017

@joshlf

This comment has been minimized.

Show comment
Hide comment
@joshlf

joshlf May 15, 2017

Contributor

Gotcha; thanks!

Contributor

joshlf commented May 15, 2017

Gotcha; thanks!

bors added a commit that referenced this issue May 15, 2017

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

As suggested by #41758.

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

bors added a commit that referenced this issue May 16, 2017

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

As suggested by #41758.
@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jan 17, 2018

Member

@rust-lang/libs Nominating for stabilization. The only potential concern is that we can generalize this to take a closure (see below). I don't know that it'd be all that useful, though, so I'm inclined to say that we shouldn't do so, and Clone and Default are the two "producing" traits in std, so standardizing around them seems logical.

fn resize_with(&mut self, new_len: usize, f: F)
where F: FnMut() -> T
Member

Mark-Simulacrum commented Jan 17, 2018

@rust-lang/libs Nominating for stabilization. The only potential concern is that we can generalize this to take a closure (see below). I don't know that it'd be all that useful, though, so I'm inclined to say that we shouldn't do so, and Clone and Default are the two "producing" traits in std, so standardizing around them seems logical.

fn resize_with(&mut self, new_len: usize, f: F)
where F: FnMut() -> T
@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jan 18, 2018

Contributor

@Mark-Simulacrum I'd be willing to make a PR that adds resize_with under a separate feature flag. I think that all three are useful; resize_default would simply be documented as equivalent to resize_with(Default::default)

Contributor

clarcharr commented Jan 18, 2018

@Mark-Simulacrum I'd be willing to make a PR that adds resize_with under a separate feature flag. I think that all three are useful; resize_default would simply be documented as equivalent to resize_with(Default::default)

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jan 18, 2018

Member

I might lean more towards just having resize_with, for now at least. It's strictly more powerful and doesn't seem all that more verbose than resize_default?

Member

sfackler commented Jan 18, 2018

I might lean more towards just having resize_with, for now at least. It's strictly more powerful and doesn't seem all that more verbose than resize_default?

@joshlf

This comment has been minimized.

Show comment
Hide comment
@joshlf

joshlf Jan 18, 2018

Contributor

My concern is that resize_with hampers discoverability - you have to come across resize_with and then connect the dots to realize that you can do resize_with(Default::default). Especially for beginners (and people who are ctrl+f-ing through the docs), this will probably make it less likely for people to figure out that they can resize using the default value.

Contributor

joshlf commented Jan 18, 2018

My concern is that resize_with hampers discoverability - you have to come across resize_with and then connect the dots to realize that you can do resize_with(Default::default). Especially for beginners (and people who are ctrl+f-ing through the docs), this will probably make it less likely for people to figure out that they can resize using the default value.

@djc

This comment has been minimized.

Show comment
Hide comment
@djc

djc Jan 18, 2018

Contributor

Discoverability is pretty easily fixed by just having some documentation, though, right?

I like the power of resize_with, but I'm wondering about optimizability. Especially for Copy types (I guess this might depend on specialization), couldn't a resize_default-like API result in much faster code over something that requires calling a closure?

Contributor

djc commented Jan 18, 2018

Discoverability is pretty easily fixed by just having some documentation, though, right?

I like the power of resize_with, but I'm wondering about optimizability. Especially for Copy types (I guess this might depend on specialization), couldn't a resize_default-like API result in much faster code over something that requires calling a closure?

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jan 18, 2018

Member

You'd just use resize for Copy types, but I'd expect resize_with(Default::default) to be literally identical to resize_default. Calling a function isn't really any different from calling a closure.

Member

sfackler commented Jan 18, 2018

You'd just use resize for Copy types, but I'd expect resize_with(Default::default) to be literally identical to resize_default. Calling a function isn't really any different from calling a closure.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jan 23, 2018

Member

The @rust-lang/libs team discussed this today and feel like resize_with would be preferable.

Member

sfackler commented Jan 23, 2018

The @rust-lang/libs team discussed this today and feel like resize_with would be preferable.

@djc

This comment has been minimized.

Show comment
Hide comment
@djc

djc Apr 1, 2018

Contributor

I've submitted #49559 implementing Vec::resize_with() in order to help move this forward.

Contributor

djc commented Apr 1, 2018

I've submitted #49559 implementing Vec::resize_with() in order to help move this forward.

djc added a commit to djc/rust that referenced this issue Apr 1, 2018

djc added a commit to djc/rust that referenced this issue Apr 2, 2018

djc added a commit to djc/rust that referenced this issue Apr 3, 2018

kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018

Rollup merge of #49559 - djc:resize-with, r=TimNN
Introduce Vec::resize_with method (see #41758)

In #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.

bors added a commit that referenced this issue Apr 4, 2018

Auto merge of #49642 - kennytm:rollup, r=kennytm
Rollup of 20 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)
 - #49623 (update mdbook)
 - #49637 (Stabilize parent_id())
 - #49639 (Update Cargo)

Failed merges:

bors added a commit that referenced this issue Apr 4, 2018

Auto merge of #49642 - kennytm:rollup, r=kennytm
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:
@oberien

This comment has been minimized.

Show comment
Hide comment
@oberien

oberien May 22, 2018

Contributor

Any updates on this? Is there anything blocking this issue?

Contributor

oberien commented May 22, 2018

Any updates on this? Is there anything blocking this issue?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 23, 2018

Contributor

Only concern is whether to keep resize_default now that resize_with exists. But it looks like resize_with can go in FCP regardless.

Contributor

clarcharr commented May 23, 2018

Only concern is whether to keep resize_default now that resize_with exists. But it looks like resize_with can go in FCP regardless.

@djc

This comment has been minimized.

Show comment
Hide comment
@djc

djc Jun 3, 2018

Contributor

@rust-lang/libs so this has been in nightly for two months. What's the next step?

Contributor

djc commented Jun 3, 2018

@rust-lang/libs so this has been in nightly for two months. What's the next step?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment