Iterator specialization for EscapeDefaults by ticki · Pull Request #30624 · rust-lang/rust

ticki · 2015-12-29T21:45:32Z

Part of #30520. Completes #24214

rust-highfive · 2015-12-29T21:45:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

ranma42 · 2015-12-29T22:12:55Z

Wouldn't it be convenient to fold this into the previous match?
I did something similar here: ranma42@0cdfa6d

ranma42 · 2015-12-29T22:17:23Z

Does this commit complete the tasks listed in #24214 ? AFAICT EscapeUnicode is still missing. I mentioned on the issue that have the branch https://github.com/ranma42/rust/tree/escape-iters for both EscapeDefault and EscapeUnicode, but there seemed to be no interest in merging it.

ticki · 2015-12-30T06:46:48Z

@ranma42 Interesting. It's true that it doesn't fix the EscapeUnicode. Have you tried to send a pull request?

ranma42 · 2015-12-30T09:12:16Z

I will send one soon. I had some unresolved questions, though, about how to handle code duplication and whether the methods should be #[inline]. I guess I will choose and fix as per suggestions on the PR.

ticki · 2015-12-30T11:30:13Z

@ranma42 I don't think they should be so. They're not simple enough for inlining to be worth it.

ticki · 2016-01-01T21:32:35Z

r? @aturon

ranma42 · 2016-01-02T15:58:47Z

I believe that the test should be moved to src/libcoretest/char.rs (I tested my implementations using your test, too, but I was unable to get its results as part of "make check" until I did that ;) ).

ranma42 · 2016-01-02T16:02:14Z

This sum can overflow if when n = usize::MAX and start = 1 (i.e. self.state is Char(_)).

ticki · 2016-01-12T17:36:20Z

@alexcrichton I have now fixed the comments you gave.

alexcrichton · 2016-01-14T22:23:16Z

This method may not be necessary as it can just be folded into the match above

ticki · 2016-01-15T19:19:01Z

@alexcrichton I updated it to reflect your points.

ranma42 · 2016-01-15T20:23:52Z

@alexcrichton I have a branch which specialises EscapeDefaults and EscapeUnicode and uses the offset/index trick in both; the symmetry might make it more clear why it is convenient. I plan to make the PR after this is landed (to reuse some parts of this, including the new test).

alexcrichton · 2016-01-15T21:16:38Z

Ok, we can perhaps try to unify once they both exist, but at least for now this seems more understandable to me at least.

@ticki, can you also squash the commits together? Other than that looks good to me, thanks!

ticki · 2016-01-16T07:47:02Z

@alexcrichton Yep. Will do.

ticki · 2016-01-16T07:48:42Z

Squash done!

alexcrichton · 2016-01-16T08:02:10Z

Looks like some other snuck in by accident?

ticki · 2016-01-16T08:09:23Z

Damn. Will fix.

…t-lang#24214 Complete the tests

…e tests to libcoretest Remove unused import Fold nth() method into the match expr

ticki · 2016-01-16T08:16:49Z

@alexcrichton Fixed.

alexcrichton · 2016-01-16T18:50:51Z

@bors: r+ d026977

bors · 2016-01-16T21:38:40Z

⌛ Testing commit d026977 with merge 1f516dc...

Part of #30520. Completes #24214

bors · 2016-01-16T23:30:28Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Dec 29, 2015

ticki force-pushed the specialization branch from 99c7702 to 8012f29Compare December 29, 2015 21:47

ranma42 reviewed Dec 29, 2015
View reviewed changes

rust-highfive assigned aturon and unassigned brson Jan 1, 2016

ranma42 reviewed Jan 2, 2016
View reviewed changes

alexcrichton reviewed Jan 14, 2016
View reviewed changes

Comment threadsrc/libcore/char.rs Outdated
alexcrichtonJan 14, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method may not be necessary as it can just be folded into the match above

ticki force-pushed the specialization branch from c8d2e8b to 1dbfe53Compare January 16, 2016 07:48

ticki added 2 commits January 16, 2016 09:12

Overide methods in iterator implementation for EscapeDefault, seerus…
15a5e02
…t-lang#24214 Complete the tests

Make style more uniform, add tests for specialization of .last(), mov…
d026977
…e tests to libcoretest Remove unused import Fold nth() method into the match expr

ticki force-pushed the specialization branch from 1dbfe53 to d026977Compare January 16, 2016 08:16

bors added a commit that referenced this pull request Jan 16, 2016
Auto merge of#30624- Ticki:specialization, r=alexcrichton
1f516dc
Part of #30520. Completes #24214

bors merged commit d026977 into rust-lang:masterJan 16, 2016

ticki deleted the specialization branch January 17, 2016 12:53

ranma42 mentioned this pull request Jan 21, 2016
Implement nth, count, and last for EscapeUnicode#31049
Closed

Uh oh!

Uh oh!

Conversation

ticki commented Dec 29, 2015

Uh oh!

rust-highfive commented Dec 29, 2015

Uh oh!

ranma42Dec 29, 2015

Choose a reason for hiding this comment

Uh oh!

ranma42 commented Dec 29, 2015

Uh oh!

ticki commented Dec 30, 2015

Uh oh!

ranma42 commented Dec 30, 2015

Uh oh!

ticki commented Dec 30, 2015

Uh oh!

ticki commented Jan 1, 2016

Uh oh!

ranma42 commented Jan 2, 2016

Uh oh!

ranma42Jan 2, 2016

Choose a reason for hiding this comment

Uh oh!

ticki commented Jan 12, 2016

Uh oh!

alexcrichtonJan 14, 2016

Choose a reason for hiding this comment

Uh oh!

ticki commented Jan 15, 2016

Uh oh!

ranma42 commented Jan 15, 2016

Uh oh!

alexcrichton commented Jan 15, 2016

Uh oh!

ticki commented Jan 16, 2016

Uh oh!

ticki commented Jan 16, 2016

Uh oh!

alexcrichton commented Jan 16, 2016

Uh oh!

ticki commented Jan 16, 2016

Uh oh!

ticki commented Jan 16, 2016

Uh oh!

alexcrichton commented Jan 16, 2016

Uh oh!

bors commented Jan 16, 2016

Uh oh!

bors commented Jan 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants