Skip to content
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

Undo the Sized specialization from Iterator::nth #46074

Merged
merged 1 commit into from
Nov 19, 2017

Conversation

scottmcm
Copy link
Member

I just added this as part of #45595, but I'm now afraid there's a specialization issue with it, since I tried to add another similar specialization, and ended up getting really disturbing test failures like

thread 'iter::test_by_ref_folds' panicked at 'assertion failed: `(left == right)`
  left: `15`, 
 right: `15`', src\libcore\../libcore/tests\iter.rs:1720:4

So since this wasn't the most critical part of the change and a new beta is branching within a week, I think putting this part back to what it was before is the best option.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2017
@bluss
Copy link
Member

bluss commented Nov 18, 2017

@bors r+

I'm fine with being suspicious of Sized vs ?Sized specialization — not so long ago it was always an ICE.

Please report a bug for the problems you found.

@bors
Copy link
Contributor

bors commented Nov 18, 2017

📌 Commit cef45b3 has been approved by bluss

@kennytm kennytm 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 Nov 18, 2017
@bors
Copy link
Contributor

bors commented Nov 19, 2017

⌛ Testing commit cef45b3 with merge d8d5b61...

bors added a commit that referenced this pull request Nov 19, 2017
Undo the Sized specialization from Iterator::nth

I just added this as part of #45595, but I'm now afraid there's a specialization issue with it, since I tried to add [another similar specialization](https://github.com/rust-lang/rust/compare/master...scottmcm:faster-iter-by-ref?expand=1#diff-1398f322bc563592215b583e9b0ba936R2390), and ended up getting really disturbing test failures like
```
thread 'iter::test_by_ref_folds' panicked at 'assertion failed: `(left == right)`
  left: `15`,
 right: `15`', src\libcore\../libcore/tests\iter.rs:1720:4
```

So since this wasn't the most critical part of the change and a new beta is branching within a week, I think putting this part back to what it was before is the best option.
@bors
Copy link
Contributor

bors commented Nov 19, 2017

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

@bors bors merged commit cef45b3 into rust-lang:master Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants