Add String::split_off. #38056

clarfonthey · 2016-11-28T18:59:25Z

This seems to match up with the latest version of the collection reform, and seems useful enough to add. First pull request!

rust-highfive · 2016-11-28T18:59:38Z

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.

sfackler · 2016-11-28T19:07:07Z

cc @rust-lang/libs

aturon · 2016-11-28T19:35:13Z

LGTM.

alexcrichton · 2016-11-28T20:04:09Z

Looks good to me too. Can you add some tests as well?

Also, should the index check be <= to return empty strings as well?

clarfonthey · 2016-11-28T20:33:18Z

@alexcrichton Yes and yes. Tests added. Not sure where I should add them, although where they are now should work.

nagisa · 2016-11-28T23:53:47Z

src/libcollections/string.rs

+    #[unstable(feature = "string_split_off", issue = "0")]
+    pub fn split_off(&mut self, mid: usize) -> String {
+        assert!(self.is_char_boundary(mid));
+        assert!(mid <= self.len());


The self.is_char_boundary already returns false for mid > self.len(), so this assertion is dead.

You also probably want to use slice_error_fail rather than plain assertion, because it will result in less code in callers and perhaps a bit better message (even if you specify mid for both start and end arguments of that function).

never mind, the slice_error_fail is private to core::str.

@nagisa It appears that a few other functions in string.rs do both checks; how about I leave the redundant check in this commit and remove the redundant calls in another pull request?

Works for me.

nagisa · 2016-11-29T12:59:25Z

Merge remote-tracking branch 'u/master'

You ought to rebase your branch onto the master rather than merge it in like this.

frewsxcv · 2016-11-29T17:25:53Z

The tests you added should be moved into this file:

https://github.com/rust-lang/rust/blob/master/src/libcollectionstest/str.rs

clarfonthey · 2016-11-29T19:07:38Z

@nagisa Wasn't sure what was preferred; I saw that squashing commits wasn't preferred and assumed that rebasing wasn't either. I've rebased.

@frewsxcv Done! I assume that you meant string.rs, but I can move them into str.rs if that's indeed the right place.

alexcrichton · 2016-11-29T21:04:25Z

Looks great to me, thanks! Could you also open a new issue to track this method? If you cc me I'll tag it appropriately and then we can fill it in for the tracking issue here.

It looks like the travis build has a few errors as well?

clarfonthey · 2016-11-30T02:04:56Z

Issue made, fixed the broken test that was causing the build to fail.

dtolnay · 2016-11-30T20:17:45Z

src/libcollections/string.rs

+    /// let mut hello = String::from("Hello, World!");
+    /// let world = hello.split_off(7);
+    /// assert_eq!(s, "Hello, ");
+    /// assert_eq!(s2, "World!");


s and s2 are called hello and world.

This is what I get for copy-pasting on a pull request. >.<

alexcrichton · 2016-12-01T00:57:31Z

The doc tests may still need an update?

failures:

---- string::String::split_off_0 stdout ----
	error: use of unstable library feature 'string_split_off' (see issue #38080)
 --> <anon>:5:19
  |
5 | let world = hello.split_off(7);
  |                   ^^^^^^^^^
  |
  = help: add #![feature(string_split_off)] to the crate attributes to enable

error: aborting due to previous error(s)

thread 'string::String::split_off_0' panicked at 'Box<Any>', /checkout/src/librustc/session/mod.rs:198
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    string::String::split_off_0

Also, could you squash the commits down into one?

clarfonthey · 2016-12-01T04:24:50Z

Rebased, squashed, and the doc test should be passing now.

alexcrichton · 2016-12-01T04:31:05Z

@bors: r+

Thanks!

bors · 2016-12-01T04:31:06Z

📌 Commit cbf734f has been approved by alexcrichton

bors · 2016-12-02T21:08:23Z

⌛ Testing commit cbf734f with merge e00b9d0...

bors · 2016-12-02T21:10:25Z

💔 Test failed - auto-mac-64-opt

alexcrichton · 2016-12-02T21:10:56Z

@bors: retry

…

On Fri, Dec 2, 2016 at 1:10 PM, bors ***@***.***> wrote: 💔 Test failed - auto-mac-64-opt <https://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/11194> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38056 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95KVAX1bugjeDEQfvN5GHmNaeA9exks5rEIlEgaJpZM4K-I6O> .

Add String::split_off. This seems to match up with the latest version of the collection reform, and seems useful enough to add. First pull request!

bors · 2016-12-03T07:47:26Z

⌛ Testing commit cbf734f with merge 24175e8...

bors · 2016-12-03T10:58:58Z

Remove redundant assertion near is_char_boundary Follow-up from #38056. `is_char_boundary` already checks for `idx <= len`, so, an extra assertion is redundant.

rust-highfive assigned sfackler Nov 28, 2016

sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 28, 2016

nagisa reviewed Nov 28, 2016

View reviewed changes

clarfonthey force-pushed the master branch from 4b0f1d0 to c3a3db0 Compare November 29, 2016 19:03

clarfonthey force-pushed the master branch from 45d4f18 to 4d6f95b Compare November 29, 2016 19:08

clarfonthey mentioned this pull request Nov 30, 2016

Tracking issue for String::split_off #38080

Closed

clarfonthey force-pushed the master branch from 4d6f95b to fe6748d Compare November 30, 2016 02:04

dtolnay reviewed Nov 30, 2016

View reviewed changes

clarfonthey force-pushed the master branch from b618f4b to 08f6261 Compare November 30, 2016 21:52

clarfonthey force-pushed the master branch from 08f6261 to cbf734f Compare December 1, 2016 04:24

Add String::split_off.

cbf734f

bors added a commit that referenced this pull request Dec 3, 2016

Auto merge of #38056 - clarcharr:master, r=alexcrichton

24175e8

Add String::split_off. This seems to match up with the latest version of the collection reform, and seems useful enough to add. First pull request!

bors merged commit cbf734f into rust-lang:master Dec 3, 2016

clarfonthey mentioned this pull request Dec 3, 2016

Remove redundant assertion near is_char_boundary #38144

Merged

bors added a commit that referenced this pull request Dec 7, 2016

Auto merge of #38144 - clarcharr:redundant, r=alexcrichton

02ea82d

Remove redundant assertion near is_char_boundary Follow-up from #38056. `is_char_boundary` already checks for `idx <= len`, so, an extra assertion is redundant.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add String::split_off. #38056

Add String::split_off. #38056

clarfonthey commented Nov 28, 2016 •

edited

Loading

rust-highfive commented Nov 28, 2016

sfackler commented Nov 28, 2016

aturon commented Nov 28, 2016

alexcrichton commented Nov 28, 2016

clarfonthey commented Nov 28, 2016 •

edited

Loading

nagisa Nov 28, 2016

nagisa Nov 28, 2016 •

edited

Loading

clarfonthey Nov 29, 2016 •

edited

Loading

nagisa Nov 29, 2016

nagisa commented Nov 29, 2016

frewsxcv commented Nov 29, 2016

clarfonthey commented Nov 29, 2016 •

edited

Loading

alexcrichton commented Nov 29, 2016

clarfonthey commented Nov 30, 2016

dtolnay Nov 30, 2016

clarfonthey Nov 30, 2016

alexcrichton commented Dec 1, 2016

clarfonthey commented Dec 1, 2016

alexcrichton commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 2, 2016

bors commented Dec 2, 2016

alexcrichton commented Dec 2, 2016 via email

bors commented Dec 3, 2016

bors commented Dec 3, 2016

Add String::split_off. #38056

Add String::split_off. #38056

Conversation

clarfonthey commented Nov 28, 2016 • edited Loading

rust-highfive commented Nov 28, 2016

sfackler commented Nov 28, 2016

aturon commented Nov 28, 2016

alexcrichton commented Nov 28, 2016

clarfonthey commented Nov 28, 2016 • edited Loading

nagisa Nov 28, 2016

Choose a reason for hiding this comment

nagisa Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

clarfonthey Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

nagisa Nov 29, 2016

Choose a reason for hiding this comment

nagisa commented Nov 29, 2016

frewsxcv commented Nov 29, 2016

clarfonthey commented Nov 29, 2016 • edited Loading

alexcrichton commented Nov 29, 2016

clarfonthey commented Nov 30, 2016

dtolnay Nov 30, 2016

Choose a reason for hiding this comment

clarfonthey Nov 30, 2016

Choose a reason for hiding this comment

alexcrichton commented Dec 1, 2016

clarfonthey commented Dec 1, 2016

alexcrichton commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 2, 2016

bors commented Dec 2, 2016

alexcrichton commented Dec 2, 2016 via email

bors commented Dec 3, 2016

bors commented Dec 3, 2016

clarfonthey commented Nov 28, 2016 •

edited

Loading

clarfonthey commented Nov 28, 2016 •

edited

Loading

nagisa Nov 28, 2016 •

edited

Loading

clarfonthey Nov 29, 2016 •

edited

Loading

clarfonthey commented Nov 29, 2016 •

edited

Loading