Implement Vec::splice and String::splice (RFC 1432) #40434

mattico · 2017-03-11T02:14:41Z

RFC: rust-lang/rfcs#1432, tracking issue: #32310
A rebase of #32355 with a few more tests.

Let me know if you have any ideas for more tests.

rust-highfive · 2017-03-11T02:14:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

mattico · 2017-03-11T02:25:58Z

I suppose I should duplicate the Vec tests in String and test the return value of splice()...

durka · 2017-03-11T02:57:45Z

src/libcollections/string.rs

+    /// The given string doesn’t need to be the same length as the range.
+    ///
+    /// Note: The element range is removed even if the iterator is not
+    /// consumed until the end.


This is contradicted by the comment under "Memory safety" below.

How about

/// Note: The element range is removed when the Splice is dropped, /// even if the iterator is not consumed until the end.

Looks like this applies to Drain as well

durka · 2017-03-11T03:02:29Z

src/libcollections/vec.rs

+    /// consumed until the end.
+    ///
+    /// Note 2: It is unspecified how many elements are removed from the vector,
+    /// if the `Splice` value is leaked.


Nit: this comma is unnecessary.

durka · 2017-03-11T03:20:41Z

Yeah that sounds better!

…

On Fri, Mar 10, 2017 at 10:12 PM, Matt Ickstadt ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libcollections/string.rs <#40434 (comment)>: > @@ -1382,6 +1382,71 @@ impl String { } } + /// Creates a splicing iterator that removes the specified range in the string, + /// replaces with the given string, and yields the removed chars. + /// The given string doesn’t need to be the same length as the range. + /// + /// Note: The element range is removed even if the iterator is not + /// consumed until the end. Looks like this applies to Drain as well — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40434 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n6KJfprI9F5n14S7h1x_qaNK6TbXks5rkhEPgaJpZM4MaD4H> .

durka · 2017-03-11T03:22:51Z

Looks like Drain removes the range and tail right away, and moves the tail back at drop. So if you leak it, the tail doesn't come back.

…

On Fri, Mar 10, 2017 at 10:20 PM, Alex Burka ***@***.***> wrote: Yeah that sounds better! On Fri, Mar 10, 2017 at 10:12 PM, Matt Ickstadt ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/libcollections/string.rs > <#40434 (comment)>: > > > @@ -1382,6 +1382,71 @@ impl String { > } > } > > + /// Creates a splicing iterator that removes the specified range in the string, > + /// replaces with the given string, and yields the removed chars. > + /// The given string doesn’t need to be the same length as the range. > + /// > + /// Note: The element range is removed even if the iterator is not > + /// consumed until the end. > > Looks like this applies to Drain as well > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#40434 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAC3n6KJfprI9F5n14S7h1x_qaNK6TbXks5rkhEPgaJpZM4MaD4H> > . >

SimonSapin · 2017-03-11T09:05:00Z

Thank you for picking this up!

mattico · 2017-03-19T16:39:36Z

Let's get this landed once the beta-madness settles down 😉.

mattico · 2017-03-28T16:44:41Z

@bors r? @alexcrichton

(Feel free to reassign, I just know you're around)

alexcrichton · 2017-04-01T22:40:46Z

sorry for being slow to review @mattico! I wanted to be sure to thank you for the PR, and just wanted to give you a heads up that I'll be traveling for the next week so it'll take some time to get around to reviewing this.

mattico · 2017-04-01T22:42:35Z

No problem, @alexcrichton. This RFC has waited a year, what's another week or so 😉.

alexcrichton

Ok got a chance to look this over and it looks fantastic to me, thanks @mattico!

Just one minor nit but other than that I'd be ready to r+

alexcrichton · 2017-04-04T23:46:18Z

src/libcollections/string.rs

+impl<'a, 'b> Drop for Splice<'a, 'b> {
+    fn drop(&mut self) {
+        unsafe {
+            let vec = (*self.string).as_mut_vec();


Would it be possible for this to use Vec::splice?

Do you mean using the Vec implementation for String in general, or using Vec::splice() in the string splice drop impl?

In general it may be pretty hard due to char decoding and whatnot, but here in drop it seems like it'd be a one-liner to call the vec impl for splicing I think, right?

I'll look into it after I rebase

bors · 2017-04-06T08:41:38Z

☔ The latest upstream changes (presumably #41102) made this pull request unmergeable. Please resolve the merge conflicts.

mattico · 2017-04-10T02:17:06Z

I broke something during rebase, I'll ping you once I've fixed it.

alexcrichton · 2017-04-10T14:37:30Z

@mattico ok just ping me whenever it's green and I'll take a final pass before r+

mattico · 2017-04-10T21:41:48Z

I found these comments interesting re: sharing drop impl https://github.com/rust-lang/rust/pull/32355/files#r57401834

mattico · 2017-04-10T21:46:28Z

Note to self: add forget() test: https://github.com/rust-lang/rust/pull/32355/files#r57402156

alexcrichton · 2017-04-10T22:19:22Z

I think we don't need to worry about leaks in this case b/c we're already in a destructor and we're just dropping the splice value that we ourselves are creating, so I don't think there's leak issues? If the split iterator is leaked then I think the string is just safely truncated, right?

carols10cents · 2017-04-17T12:25:15Z

Hi @mattico! Friendly ping to keep this on your radar!

mattico · 2017-04-17T16:48:12Z

Haven't forgotten about this, just haven't had much time. Will try to get back to this this week sometime.

@SimonSapin

Implement Vec::splice and String::splice (RFC 1432) RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310 A rebase of rust-lang#32355 with a few more tests. Let me know if you have any ideas for more tests. cc @SimonSapin

mattico · 2017-04-24T14:51:25Z

Oh whoops, I just pushed a forget() test, which codifies the current behavior. Technically for Vec it's unspecified what happens if you forget the Splice, but the test will at least make sure it doesn't change behavior accidentally.

mattico · 2017-04-24T14:52:08Z

These new tests pass locally so it should be safe to r+

alexcrichton · 2017-04-24T14:55:40Z

@bors: r+

Thanks!

bors · 2017-04-24T14:55:40Z

📌 Commit feae5a0 has been approved by alexcrichton

Rollup of 3 pull requests - Successful merges: #40434, #41370, #41500 - Failed merges:

bors · 2017-04-24T17:28:24Z

⌛ Testing commit feae5a0 with merge 05a2d62...

bors · 2017-04-24T18:48:09Z

💔 Test failed - status-travis

frewsxcv · 2017-04-24T18:50:12Z

@bors retry #40474

@SimonSapin

Implement Vec::splice and String::splice (RFC 1432) RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310 A rebase of rust-lang#32355 with a few more tests. Let me know if you have any ideas for more tests. cc @SimonSapin

@SimonSapin

Implement Vec::splice and String::splice (RFC 1432) RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310 A rebase of rust-lang#32355 with a few more tests. Let me know if you have any ideas for more tests. cc @SimonSapin

@SimonSapin

Implement Vec::splice and String::splice (RFC 1432) RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310 A rebase of rust-lang#32355 with a few more tests. Let me know if you have any ideas for more tests. cc @SimonSapin

alexcrichton · 2017-04-25T05:55:58Z

@bors: retry

bors · 2017-04-25T10:34:13Z

⌛ Testing commit feae5a0 with merge c7e724a...

@SimonSapin

Implement Vec::splice and String::splice (RFC 1432) RFC: rust-lang/rfcs#1432, tracking issue: #32310 A rebase of #32355 with a few more tests. Let me know if you have any ideas for more tests. cc @SimonSapin

bors · 2017-04-25T13:33:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c7e724a to master...

rust-highfive assigned sfackler Mar 11, 2017

mattico force-pushed the splice-update branch from 16e83ff to fae21f7 Compare March 11, 2017 02:44

durka reviewed Mar 11, 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 Mar 14, 2017

mattico changed the title ~~Implement Vec::splice and String::splice~~ Implement Vec::splice and String::splice (RFC 1432) Mar 22, 2017

rust-highfive assigned alexcrichton and unassigned sfackler Mar 28, 2017

nagisa mentioned this pull request Apr 1, 2017

Feature request: Vec::insert_slice rust-lang/rfcs#1964

Closed

alexcrichton reviewed Apr 4, 2017

View reviewed changes

mattico force-pushed the splice-update branch 2 times, most recently from 50cfe26 to 9878aa0 Compare April 8, 2017 21:48

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

frewsxcv mentioned this pull request Apr 24, 2017

Rollup of 3 pull requests #41503

Closed

Add Splice forget test

feae5a0

bors added a commit that referenced this pull request Apr 24, 2017

Auto merge of #41503 - frewsxcv:rollup, r=frewsxcv

38c99d2

Rollup of 3 pull requests - Successful merges: #40434, #41370, #41500 - Failed merges:

frewsxcv mentioned this pull request Apr 24, 2017

Rollup of 7 pull requests #41513

Closed

frewsxcv mentioned this pull request Apr 24, 2017

Rollup of 6 pull requests #41516

Closed

frewsxcv mentioned this pull request Apr 25, 2017

Rollup of 9 pull requests #41522

Closed

arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2017

bors merged commit feae5a0 into rust-lang:master Apr 25, 2017

mattico deleted the splice-update branch April 28, 2017 02:02

brson mentioned this pull request Jun 6, 2017

Rust 1.19 regression, odds 0.2.25, splice method #42462

Closed

Implement Vec::splice and String::splice (RFC 1432) #40434

Implement Vec::splice and String::splice (RFC 1432) #40434

Conversation

mattico commented Mar 11, 2017

rust-highfive commented Mar 11, 2017

mattico commented Mar 11, 2017

durka Mar 11, 2017

Choose a reason for hiding this comment

mattico Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

mattico Mar 11, 2017

Choose a reason for hiding this comment

durka Mar 11, 2017

Choose a reason for hiding this comment

durka commented Mar 11, 2017 via email

durka commented Mar 11, 2017 via email

SimonSapin commented Mar 11, 2017

mattico commented Mar 19, 2017

mattico commented Mar 28, 2017

alexcrichton commented Apr 1, 2017

mattico commented Apr 1, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Apr 4, 2017

Choose a reason for hiding this comment

mattico Apr 5, 2017

Choose a reason for hiding this comment

alexcrichton Apr 5, 2017

Choose a reason for hiding this comment

mattico Apr 8, 2017

Choose a reason for hiding this comment

bors commented Apr 6, 2017

mattico commented Apr 10, 2017

alexcrichton commented Apr 10, 2017

mattico commented Apr 10, 2017

mattico commented Apr 10, 2017

alexcrichton commented Apr 10, 2017

carols10cents commented Apr 17, 2017

mattico commented Apr 17, 2017

mattico commented Apr 24, 2017

mattico commented Apr 24, 2017

alexcrichton commented Apr 24, 2017

bors commented Apr 24, 2017

bors commented Apr 24, 2017

bors commented Apr 24, 2017

frewsxcv commented Apr 24, 2017

alexcrichton commented Apr 25, 2017

bors commented Apr 25, 2017

bors commented Apr 25, 2017

mattico Mar 11, 2017 •

edited

Loading