Add repeat method on slice #48999

GuillaumeGomez · 2018-03-13T23:16:46Z

rust-highfive · 2018-03-13T23:16:50Z

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

sinkuu · 2018-03-14T03:55:55Z

src/liballoc/slice.rs

+
+        // `2^expn` repetition is done by doubling `buf` `expn`-times.
+        buf.extend(self);
+


You have missed this part: https://github.com/sinkuu/rust/blob/3d58543d49266a7ec3eb5f5f2ffaf902fce17c53/src/liballoc/str.rs#L2084-L2102

sinkuu · 2018-03-14T03:59:02Z

Can you replace the implementation of str::repeat with String::from_utf8_unchecked(self.as_bytes().repeat(n)) to avoid code duplication?

GuillaumeGomez · 2018-03-14T08:56:13Z

@sinkuu: That's what I thought about doing but they seem to make specific str operations (checking if they're at the end of a char, etc...). Did I misunderstood?

sinkuu · 2018-03-14T09:34:15Z

@GuillaumeGomez Valid Strings must have char boundary at their ends, so repeating them does not require special considerations.
Just like String::push_str simply extends their internal Vec<u8> with the other.
https://doc.rust-lang.org/1.22.0/src/alloc/string.rs.html#804-808

scottmcm · 2018-03-15T01:49:49Z

src/liballoc/slice.rs

+
+    /// Creates a [`Vec`] by repeating a slice `n` times.
+    ///
+    /// [`Vec`]: ../../std/vec/struct.Vec.html


[01:26:06] std/primitive.slice.html:1018: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/vec/struct.Vec.html [01:26:20] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9

Kimundi · 2018-03-15T09:08:27Z

src/liballoc/slice.rs

+        // first `rem` repetitions from `buf` itself.
+        let rem_len = self.len() * n - buf.len(); // `self.len() * rem`
+        if rem_len > 0 {
+            // `buf.extend(buf[0 .. rem_len])`:


Hm, why is the code here not just literally buf.extend(&buf[..rem_len])? Worse codegen?

Because you cannot have &mut buf and &buf at the same time. The comment was meant to be like a pseudocode.

Ah, of course. I somehow forgot about the borrow checker for a minute there, too much C++ recently...

Kimundi · 2018-03-15T09:10:38Z

src/liballoc/slice.rs

+        // `n = 2^expn + rem (2^expn > rem, expn >= 0, rem >= 0)`.
+        // `2^expn` is the number represented by the leftmost '1' bit of `n`,
+        // and `rem` is the remaining part of `n`.
+


It took me a few minutes to figure out why this function is so complex - maybe add a comment that explicitly explains that its trying to minimize the copy_nonoverlapping calls by always copying as large chunks as possible?

I just moved the code around, didn't modify it. It's exactly like it was before in str.

GuillaumeGomez · 2018-03-17T22:28:25Z

It passed tests. Is there anything else I should do?

Kimundi · 2018-03-18T12:02:18Z

src/liballoc/slice.rs

+    /// ```
+    /// assert_eq!([1, 2].repeat(3), vec![1, 2, 1, 2, 1, 2]);
+    /// ```
+    #[stable(feature = "repeat_slice", since = "1.27.0")]


All new APIs should start off as unstable. Just invent a new feature name for it, like "repeat_generic_slice" or so.

GuillaumeGomez · 2018-03-18T19:38:31Z

Updated.

Kimundi · 2018-03-19T13:26:22Z

Ah, sorry I didn't notice their absence before, but if you could add some basic tests that check that repeat works as expected on a few different slice types that would be good. (Even if its the same code, just moved around, there might always be something unexpected going on)

GuillaumeGomez · 2018-03-19T13:45:26Z

Sure. I'll add them later in the week.

GuillaumeGomez · 2018-03-27T15:09:51Z

Added a test.

scottmcm

BTW, I mentioned this on IRC yesterday, and someone tried it out and got really excited that it was like 90x faster than what they were doing before 😄

scottmcm · 2018-03-27T18:40:41Z

src/liballoc/slice.rs

+    ///
+    /// Basic usage:
+    ///
+    /// ```


[01:10:05] ---- slice.rs - slice::[T]::repeat (line 1732) stdout ---- [01:10:05] error[E0658]: use of unstable library feature 'repeat_generic_slice': it's on str, why not on slice? (see issue #48784) [01:10:05] --> slice.rs:1733:19 [01:10:05] | [01:10:05] 4 | assert_eq!([1, 2].repeat(3), vec![1, 2, 1, 2, 1, 2]); [01:10:05] | ^^^^^^ [01:10:05] | [01:10:05] = help: add #![feature(repeat_generic_slice)] to the crate attributes to enable

GuillaumeGomez · 2018-03-28T08:54:51Z

Fixed doc example.

GuillaumeGomez · 2018-04-04T22:15:29Z

cc @Kimundi

pietroalbini · 2018-04-09T10:01:33Z

Ping from triage @Kimundi! The author pushed new commits, can you review this PR again?

GuillaumeGomez · 2018-04-13T21:17:13Z

Anyone?

pietroalbini · 2018-04-13T21:25:34Z

Can someone from @rust-lang/libs review this PR? Thanks!

pietroalbini · 2018-04-23T10:57:33Z

Ping from triage! Can @Kimundi (or someone else from @rust-lang/libs) review this?

Kimundi · 2018-04-23T11:01:00Z

@bors: r+

bors · 2018-04-23T11:01:01Z

📌 Commit 3c1fea9 has been approved by Kimundi

bors · 2018-04-23T13:07:44Z

⌛ Testing commit 3c1fea9 with merge bc1114ed3ed5e533ce9d4a1efb807e9753fdc38d...

bors · 2018-04-23T14:44:07Z

💔 Test failed - status-travis

GuillaumeGomez · 2018-04-23T21:57:50Z

I can't see which test failed. Just that we got a signal 11...

Mark-Simulacrum · 2018-04-24T03:02:38Z

Possibly a resurgence of #49775? cc @alexcrichton

But probably a spurious failure, seems somewhat unlikely to be caused by these changes.

@bors retry

bors · 2018-04-24T03:31:18Z

⌛ Testing commit 3c1fea9 with merge f305b02...

Add repeat method on slice Fixes #48784.

bors · 2018-04-24T05:43:26Z

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

rust-highfive assigned Kimundi Mar 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2018

sinkuu reviewed Mar 14, 2018

View reviewed changes

GuillaumeGomez force-pushed the add-repeat-on-slice branch from 715de99 to 3a1a6a7 Compare March 14, 2018 18:42

scottmcm reviewed Mar 15, 2018

View reviewed changes

Kimundi reviewed Mar 15, 2018

View reviewed changes

GuillaumeGomez force-pushed the add-repeat-on-slice branch 2 times, most recently from 4fc6276 to 234375b Compare March 15, 2018 23:14

Kimundi suggested changes Mar 18, 2018

View reviewed changes

GuillaumeGomez force-pushed the add-repeat-on-slice branch from 234375b to 5ec2a43 Compare March 18, 2018 19:38

GuillaumeGomez force-pushed the add-repeat-on-slice branch from 5ec2a43 to 17801c6 Compare March 19, 2018 13:21

shepmaster 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 Mar 23, 2018

GuillaumeGomez force-pushed the add-repeat-on-slice branch from 17801c6 to d8863d7 Compare March 27, 2018 14:53

scottmcm reviewed Mar 27, 2018

View reviewed changes

GuillaumeGomez added 2 commits March 28, 2018 09:02

Add repeat method on slice

1ef70a0

Add run-pass test for repeat-generic-slice feature

3c1fea9

GuillaumeGomez force-pushed the add-repeat-on-slice branch from d8863d7 to 3c1fea9 Compare March 28, 2018 07:02

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

frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 16, 2018

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 23, 2018

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2018

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

Auto merge of #48999 - GuillaumeGomez:add-repeat-on-slice, r=Kimundi

f305b02

Add repeat method on slice Fixes #48784.

bors merged commit 3c1fea9 into rust-lang:master Apr 24, 2018

GuillaumeGomez deleted the add-repeat-on-slice branch April 24, 2018 08:42

kennytm mentioned this pull request Oct 27, 2018

repeat method for (byte) slices? #48784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repeat method on slice #48999

Add repeat method on slice #48999

GuillaumeGomez commented Mar 13, 2018

rust-highfive commented Mar 13, 2018

sinkuu Mar 14, 2018

sinkuu commented Mar 14, 2018

GuillaumeGomez commented Mar 14, 2018

sinkuu commented Mar 14, 2018

scottmcm Mar 15, 2018

Kimundi Mar 15, 2018

sinkuu Mar 15, 2018

Kimundi Mar 15, 2018

Kimundi Mar 15, 2018

GuillaumeGomez Mar 15, 2018

GuillaumeGomez commented Mar 17, 2018

Kimundi Mar 18, 2018

GuillaumeGomez Mar 18, 2018

GuillaumeGomez commented Mar 18, 2018

Kimundi commented Mar 19, 2018 •

edited

Loading

GuillaumeGomez commented Mar 19, 2018

GuillaumeGomez commented Mar 27, 2018

scottmcm left a comment

scottmcm Mar 27, 2018

GuillaumeGomez commented Mar 28, 2018

GuillaumeGomez commented Apr 4, 2018

pietroalbini commented Apr 9, 2018

GuillaumeGomez commented Apr 13, 2018

pietroalbini commented Apr 13, 2018

pietroalbini commented Apr 23, 2018

Kimundi commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

GuillaumeGomez commented Apr 23, 2018

Mark-Simulacrum commented Apr 24, 2018

bors commented Apr 24, 2018

bors commented Apr 24, 2018


		// `2^expn` repetition is done by doubling `buf` `expn`-times.
		buf.extend(self);

Add repeat method on slice #48999

Add repeat method on slice #48999

Conversation

GuillaumeGomez commented Mar 13, 2018

rust-highfive commented Mar 13, 2018

Choose a reason for hiding this comment

sinkuu commented Mar 14, 2018

GuillaumeGomez commented Mar 14, 2018

sinkuu commented Mar 14, 2018

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

GuillaumeGomez commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 18, 2018

Kimundi commented Mar 19, 2018 • edited Loading

GuillaumeGomez commented Mar 19, 2018

GuillaumeGomez commented Mar 27, 2018

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 28, 2018

GuillaumeGomez commented Apr 4, 2018

pietroalbini commented Apr 9, 2018

GuillaumeGomez commented Apr 13, 2018

pietroalbini commented Apr 13, 2018

pietroalbini commented Apr 23, 2018

Kimundi commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

bors commented Apr 23, 2018

GuillaumeGomez commented Apr 23, 2018

Mark-Simulacrum commented Apr 24, 2018

bors commented Apr 24, 2018

bors commented Apr 24, 2018

Kimundi commented Mar 19, 2018 •

edited

Loading