Skip to content

/rust

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

Rewrite/expand doc examples for `Vec::set_len`. #34911

Merged
merged 1 commit into from Jul 21, 2016

Conversation

Reviewers
No reviews
Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@frewsxcv @steveklabnik @bors @alexcrichton @malbarbo
@frewsxcv
Copy link
Member

frewsxcv commented Jul 19, 2016

No description provided.

src/libcollections/vec.rs Outdated
@@ -532,9 +532,33 @@ impl<T> Vec<T> {
/// # Examples
///
/// ```
/// let mut v = vec![1, 2, 3, 4];
/// use std::ptr::drop_in_place;

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

idiom is to import the module, so just std::ptr...
src/libcollections/vec.rs Outdated
/// unsafe {
/// v.set_len(1);
/// drop_in_place(&mut vec[3]);

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

... and then use ptr::drop_in_place here
src/libcollections/vec.rs Outdated
/// ```
///
/// In this example, there is a memory leak since the memory locations
/// owned by the vector were not freed prior to the `set_len` called:

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

"call" not "called"
src/libcollections/vec.rs Outdated
@@ -532,9 +532,33 @@ impl<T> Vec<T> {
/// # Examples
///
/// ```
/// let mut v = vec![1, 2, 3, 4];
/// use std::ptr::drop_in_place;
/// let mut vec = vec!['r', 'u', 's', 't'];

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

could you put an extra space before and after this?
src/libcollections/vec.rs Outdated
/// owned by the vector were not freed prior to the `set_len` called:
///
/// ```
/// let mut vec = vec!['r', 'u', 's', 't'];

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

could you put a space after this line, please?
src/libcollections/vec.rs Outdated
/// values of unallocated memory:
///
/// ```
/// let mut vec = Vec::new();

This comment has been minimized.

Sign in to view
Copy link
@steveklabnik

steveklabnik Jul 19, 2016

Member

could you put a space after this?
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Jul 19, 2016

I have a bunch of teeny nitpicks, but this is great! Thanks so much. r=me after they're fixed

@frewsxcv frewsxcv force-pushed the frewsxcv:vec-set-len branch 2 times, most recently from 231227b to 102419a Jul 19, 2016

@frewsxcv

This comment has been minimized.

Sign in to view
Copy link
Member Author

frewsxcv commented Jul 19, 2016

Comments have been addressed. Thanks for the review!
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Jul 19, 2016

@bors: r+ rollup

thanks so much!!!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 102419a has been approved by steveklabnik
@frewsxcv

This comment has been minimized.

Sign in to view
Copy link
Member Author

frewsxcv commented Jul 19, 2016 

failures:
---- vec::Vec<T>::set_len_0 stdout ----
    <anon>:10:5: 10:6 error: unresolved name `v` [E0425]
<anon>:10     v.set_len(3);
              ^
error: aborting due to previous error(s) 
thread 'vec::Vec<T>::set_len_0' panicked at 'Box<Any>', src/librustc/session/mod.rs:171
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- vec::Vec<T>::set_len_2 stdout ----
    <anon>:4:19: 4:27 error: unable to infer enough type information about `_`; type annotations or generic parameter binding required [E0282]
<anon>:4     let mut vec = Vec::new();
                           ^~~~~~~~
error: aborting due to previous error(s) 
thread 'vec::Vec<T>::set_len_2' panicked at 'Box<Any>', src/librustc/session/mod.rs:171
failures:
    vec::Vec<T>::set_len_0
    vec::Vec<T>::set_len_2

@frewsxcv frewsxcv force-pushed the frewsxcv:vec-set-len branch from 102419a to a005b2c Jul 19, 2016

@frewsxcv

This comment has been minimized.

Sign in to view
Copy link
Member Author

frewsxcv commented Jul 19, 2016

Changes made:

@@ -538,7 +554,7 @@ impl<T> Vec<T> {
     ///
     /// unsafe {
     ///     ptr::drop_in_place(&mut vec[3]);
-    ///     v.set_len(3);
+    ///     vec.set_len(3);
     /// }
     /// assert_eq!(vec, ['r', 'u', 's']);
     /// ```
@@ -559,7 +575,7 @@ impl<T> Vec<T> {
     /// values of unallocated memory:
     ///
     /// ```
-    /// let mut vec = Vec::new();
+    /// let mut vec: Vec<char> = Vec::new();
     ///
     /// unsafe {
     ///     vec.set_len(4);

r? @steveklabnik

@rust-highfive rust-highfive assigned steveklabnik Jul 19, 2016

@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Jul 19, 2016

ah, nice catch.

@bors: r+ rollup
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit a005b2c has been approved by steveklabnik
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 20, 2016

⌛️ Testing commit a005b2c with merge afaaf4b...

bors added a commit that referenced this pull request Jul 20, 2016

@bors
Auto merge of #34911 - frewsxcv:vec-set-len, r=steveklabnik 
Rewrite/expand doc examples for `Vec::set_len`.

None
afaaf4b
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jul 20, 2016

@bors: retry

On Tue, Jul 19, 2016 at 10:01 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1834

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34911 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95DexKIaR5TmEMW3LFnrxVZpVNTSeks5qXauegaJpZM4JPUUu
.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 20, 2016

@GuillaumeGomez
Rollup merge of rust-lang#34911 - frewsxcv:vec-set-len, r=steveklabnik 
Rewrite/expand doc examples for `Vec::set_len`.

None
89751d8

@GuillaumeGomez GuillaumeGomez referenced this pull request Jul 20, 2016

Closed

Rollup of 8 pull requests #34936

bors added a commit that referenced this pull request Jul 20, 2016

@bors
Auto merge of #34936 - GuillaumeGomez:rollup, r=GuillaumeGomez 
Rollup of 8 pull requests

- Successful merges: #34854, #34855, #34880, #34895, #34911, #34916, #34921, #34930
- Failed merges: #33951, #34850
90474f3

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 20, 2016

@GuillaumeGomez
Rollup merge of rust-lang#34911 - frewsxcv:vec-set-len, r=steveklabnik 
Rewrite/expand doc examples for `Vec::set_len`.

None
9ee53ff

@GuillaumeGomez GuillaumeGomez referenced this pull request Jul 20, 2016

Merged

Rollup of 7 pull requests #34939

bors added a commit that referenced this pull request Jul 20, 2016

@bors
Auto merge of #34939 - GuillaumeGomez:rollup, r=GuillaumeGomez 
Rollup of 7 pull requests

- Successful merges: #34854, #34855, #34880, #34895, #34911, #34921, #34930
- Failed merges: #33951, #34850
c1bb843
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 21, 2016

⌛️ Testing commit a005b2c with merge 3dcdb53...

bors added a commit that referenced this pull request Jul 21, 2016

@bors
Auto merge of #34911 - frewsxcv:vec-set-len, r=steveklabnik 
Rewrite/expand doc examples for `Vec::set_len`.

None
3dcdb53
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jul 21, 2016

@bors: retry

On Wed, Jul 20, 2016 at 6:27 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1202

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34911 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95JWDGJcx0HSRDPWp1M54-OeXoUy1ks5qXsrygaJpZM4JPUUu
.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 21, 2016

@GuillaumeGomez
Rollup merge of rust-lang#34911 - frewsxcv:vec-set-len, r=steveklabnik 
Rewrite/expand doc examples for `Vec::set_len`.

None
27876c0

bors added a commit that referenced this pull request Jul 21, 2016

@bors
Auto merge of #34939 - GuillaumeGomez:rollup, r=GuillaumeGomez 
Rollup of 7 pull requests

- Successful merges: #34854, #34855, #34880, #34895, #34911, #34921, #34930
- Failed merges: #33951, #34850
7588653

@bors bors merged commit a005b2c into rust-lang:master Jul 21, 2016
1 of 2 checks passed

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@malbarbo

This comment has been minimized.

Sign in to view
Copy link
Contributor

malbarbo commented on src/libcollections/vec.rs in a005b2c Jul 21, 2016

There is no leak here. char is a copy type, so there is no missing drop call. You can get a leak with vec![vec![2], ...]

This comment has been minimized.

Sign in to view
Copy link
Member Author

frewsxcv replied Jul 21, 2016

You're absolutely right. Do you want to open a pull request to fix the example? If not, I can do it.

This comment has been minimized.

Sign in to view
Copy link
Contributor

malbarbo replied Jul 22, 2016

Please, go ahead and fix it.

This comment has been minimized.

Sign in to view
Copy link
Member Author

frewsxcv replied Jul 23, 2016

Opened a PR: #34989

@frewsxcv frewsxcv referenced this pull request Jul 23, 2016

Merged

Fix incorrect 'memory leak' example for `Vec::set_len`. #34989

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jul 23, 2016

@frewsxcv
Fix incorrect 'memory leak' example for `Vec::set_len`. 
Example was written in rust-lang#34911

Issue was brought up in this comment:

rust-lang@a005b2c#commitcomment-18346958
7c566ff

@frewsxcv frewsxcv deleted the frewsxcv:vec-set-len branch Jul 23, 2016

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jul 23, 2016

@frewsxcv
Fix incorrect 'memory leak' example for `Vec::set_len`. 
Example was written in rust-lang#34911

Issue was brought up in this comment:

rust-lang@a005b2c#commitcomment-18346958
676659a

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jul 23, 2016

@frewsxcv
Fix incorrect 'memory leak' example for `Vec::set_len`. 
Example was written in rust-lang#34911

Issue was brought up in this comment:

rust-lang@a005b2c#commitcomment-18346958
1e0043e

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 24, 2016

@Manishearth
Rollup merge of rust-lang#34989 - frewsxcv:fix-set-len-doc-example, r… 
…=nagisa

Fix incorrect 'memory leak' example for `Vec::set_len`.

Example was written in rust-lang#34911

Issue was brought up in this comment:

rust-lang@a005b2c#commitcomment-18346958
52c293c

knight42 added a commit to knight42/rust that referenced this pull request Jul 26, 2016

@frewsxcv @knight42
Fix incorrect 'memory leak' example for `Vec::set_len`. 
Example was written in rust-lang#34911

Issue was brought up in this comment:

rust-lang@a005b2c#commitcomment-18346958
0c43bd0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.