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.

Sign up for GitHub

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

Jump to bottom

Remove IteratorExt #23300

Merged
merged 1 commit into from
Mar 29, 2015
Merged

Remove IteratorExt #23300

merged 1 commit into from
Mar 29, 2015

Conversation

sfackler
Copy link
Member

All methods are inlined into Iterator with Self: Sized bounds to make
sure Iterator is still object safe.

[breaking-change]

This is blocked on ICEs: https://gist.github.com/sfackler/5aff7c57cf8d896e2c6f
Seem to be similar to #23281.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

alexcrichton
alexcrichton reviewed Mar 12, 2015
View reviewed changes
src/libcore/iter.rs
@@ -200,7 +147,7 @@ pub trait IteratorExt: Iterator + Sized {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn nth(&mut self, mut n: usize) -> Option<Self::Item> {
fn nth(&mut self, mut n: usize) -> Option<Self::Item> where Self: Sized {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that functions like this which can relax the Self: Sized bound are ok to. In theory an iterator could override this method to provide a specialized implementation.

@alexcrichton
Copy link
Member

I'm ok merging this as-is basically (nit or not), so r=me once the ICE is fixed

@bors
Copy link
Contributor

bors commented Mar 16, 2015

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

@nikomatsakis
Copy link
Contributor

@sfackler I'm investigating ICE now...

@nikomatsakis
Copy link
Contributor

OK, I... sort of understand the ICE. The problem is constructing the vtable for an Iterator object when the type in question may not satisfy the where clauses. I'll have to poke at this tomorrow, there may be multiple bugs here, not sure at this moment of the best fix, but seems eminently solvable. :)

@nikomatsakis nikomatsakis mentioned this pull request Mar 17, 2015
Closed
@nikomatsakis
Copy link
Contributor

see #23435 for a smaller example and explanation of the ICE

@nikomatsakis
Copy link
Contributor

#23438 should fix the ICE

@sfackler
Copy link
Member Author

Awesome, thanks Niko!

@sfackler
Copy link
Member Author

Rebased over @nikomatsakis's fix, but it looks like we're still running into a very similar crash: https://gist.github.com/sfackler/6bf9e4140c1890553528

@sfackler
Copy link
Member Author

Backtrace:

stack backtrace:
   1:     0x7f4dcc8166e4 - sys::backtrace::write::hb2ecc26860cf26a5N0C
   2:     0x7f4dcc83df90 - panicking::on_panic::h960f9523a4558f10fhJ
   3:     0x7f4dcc772f4a - rt::unwind::begin_unwind_inner::h6b71715c6cd07544OXI
   4:     0x7f4dc9c357bd - rt::unwind::begin_unwind::h10330668448694945308
   5:     0x7f4dc9c35763 - diagnostic::SpanHandler::span_bug::ha94d4327955c6b76FaB
   6:     0x7f4dca4c1053 - session::Session::span_bug::ha063ab5cf89afb7arTn
   7:     0x7f4dcbfa18d5 - trans::monomorphize::normalize_associated_type::h15578180277037779921
   8:     0x7f4dcbf9cec5 - trans::meth::emit_vtable_methods::closure.46262
   9:     0x7f4dcbf9c230 - vec::Vec<T>.FromIterator<T>::from_iter::h14757000865658806530
  10:     0x7f4dcbf9a8c1 - trans::meth::emit_vtable_methods::h869b133958115c379By
  11:     0x7f4dcbf9ba5a - iter::FlatMap<I, U, F>.Iterator::next::closure.46232
  12:     0x7f4dcbf9b6e8 - iter::FlatMap<I, U, F>.Iterator::next::h18664991822273943
  13:     0x7f4dcbf9afcc - vec::Vec<T>.FromIterator<T>::from_iter::h1954666775356190594
  14:     0x7f4dcbedc3eb - trans::meth::get_vtable::hcdbf30f6716b5c22Svy
  15:     0x7f4dcbed7f12 - trans::expr::unsized_info::h1026628279121771536
  16:     0x7f4dcbee34c3 - trans::expr::apply_adjustments::apply_autoref::hfb6d70e6aeef5555Txh
  17:     0x7f4dcbe9076d - trans::expr::trans::hbd4f36981c222183ifh
  18:     0x7f4dcbe8e3e4 - trans::expr::trans_into::h2fbdf169c8afb1f608g
  19:     0x7f4dcbed769c - trans::expr::trans_rvalue_stmt_unadjusted::h0b4eefee561d46b3wgi
  20:     0x7f4dcbe8e9bd - trans::expr::trans_into::h2fbdf169c8afb1f608g
  21:     0x7f4dcbe8da34 - trans::controlflow::trans_stmt_semi::hf010868e8995b5e144d
  22:     0x7f4dcbe8f510 - trans::controlflow::trans_block::h1a3e1fe31adb4417R5d
  23:     0x7f4dcbf5739b - trans::base::trans_closure::h3785eaa409045c3cv7s
  24:     0x7f4dcbe793f8 - trans::base::trans_fn::hb17787e2baa85f80oit
  25:     0x7f4dcbe74f31 - trans::base::trans_item::h9d97a4e78d8fd135gGt
  26:     0x7f4dcbf5efcc - trans::base::trans_crate::h317d2891eb44f3c8KCu
  27:     0x7f4dccea22a3 - driver::phase_4_translate_to_llvm::hf105818bf70a091fmOa
  28:     0x7f4dcce7cee9 - driver::compile_input::ha7a5cd31f19269c3Rba
  29:     0x7f4dccf357b2 - run_compiler::hffd60a8ae6171fbbz2b
  30:     0x7f4dccf335ec - thunk::F.Invoke<A, R>::invoke::h17211452014221736608
  31:     0x7f4dccf327a9 - rt::unwind::try::try_fn::h10298546463733922815
  32:     0x7f4dcc8bc118 - rust_try_inner
  33:     0x7f4dcc8bc105 - rust_try
  34:     0x7f4dccf32b29 - thunk::F.Invoke<A, R>::invoke::h16798335321014197657
  35:     0x7f4dcc82b0d8 - sys::thread::create::thread_start::h870bf041dfc81c1bDQH
  36:     0x7f4dc67ba373 - start_thread
  37:     0x7f4dcc3e927c - __clone
  38:                0x0 - <unknown>

@nikomatsakis
Copy link
Contributor

@sfackler hmm! I'll check it out again...

@nikomatsakis
Copy link
Contributor

@sfackler ok I have a fix for the ICE now, but I don't have a good standalone test case yet. The problem seems to arise because I was previously trying to normalize first, when normalization itself can encounter errors.

@nikomatsakis nikomatsakis mentioned this pull request Mar 18, 2015
Closed
@bors
Copy link
Contributor

bors commented Mar 19, 2015

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

@sfackler
Copy link
Member Author

This works rebased on top of #23486 so it should be good to go once that merges.

apasel422
apasel422 reviewed Mar 22, 2015
View reviewed changes
src/libcore/iter.rs
@@ -928,7 +885,7 @@ pub trait IteratorExt: Iterator + Sized {
/// `std::usize::MAX` elements of the original iterator.
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn rev(self) -> Rev<Self> {
fn rev(self) -> Rev<Self> where Self: Sized {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this where Self: Sized + DoubleEndedIterator in order to fix #23587?

@sfackler sfackler changed the title [WIP] Remove IteratorExt Remove IteratorExt Mar 28, 2015
@sfackler
Copy link
Member Author

@bors r=alexcrichton 11f5ac6

@bors
Copy link
Contributor

bors commented Mar 28, 2015

⌛ Testing commit 11f5ac6 with merge 2f4bad9...

@bors
Copy link
Contributor

bors commented Mar 28, 2015

💔 Test failed - auto-win-64-nopt-t

@sfackler
Remove IteratorExt
d502f42 
All methods are inlined into Iterator with `Self: Sized` bounds to make
sure Iterator is still object safe.

[breaking-change]
@sfackler
Copy link
Member Author

@bors: r=alexcrichton d502f42

@bors
Copy link
Contributor

bors commented Mar 28, 2015

⌛ Testing commit d502f42 with merge c89de2c...

bors added a commit that referenced this pull request Mar 28, 2015
@bors
Auto merge of #23300 - sfackler:rm-iterator-ext, r=alexcrichton
c89de2c 
All methods are inlined into Iterator with `Self: Sized` bounds to make
sure Iterator is still object safe.

[breaking-change]

This is blocked on ICEs: https://gist.github.com/sfackler/5aff7c57cf8d896e2c6f
Seem to be similar to #23281.
@bors
Copy link
Contributor

bors commented Mar 29, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@bors bors merged commit d502f42 into rust-lang:master Mar 29, 2015
@bors bors mentioned this pull request Mar 29, 2015
Merged
@sfackler sfackler deleted the rm-iterator-ext branch November 26, 2016 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sfackler @rust-highfive @alexcrichton @bors @nikomatsakis @apasel422