Work-around optimiser deficiencies in Range iterator.

[huonw]

The conditional mutation of the previous implementation resulted in poor
code, making it unconditional makes Range less well behaved as an
Iterator (but still legal) but also makes it fast.

The intention is that this change will be reverted when rustc/LLVM
handle the best-behaved implementation better.

cc #24660

[rust-highfive]

r? @aturon

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

[aturon]

Hm, I'm worried about cases where this causes overflow and you have debug assertions enabled (whereas the current code avoids the overflow).

[huonw]

I believe that will only trigger after None has been yielded at least once, so is OK per the "behaviour after None is unspecified" part of Iterator, however, I agree that's a little worrying.

[aturon]

Oh yes, of course, my apologies for not reading the commentary more carefully.

@bors: r+

[bors]

📌 Commit 97b7c76 has been approved by aturon

[bors]

⌛ Testing commit 97b7c76 with merge 4e9399c...

[bors]

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

[pnkfelix]

@huonw any idea about that build failure?

rustc: x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcollections
rustc: x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libstd/path.rs:680:13: 680:14 error: unreachable pattern [E0001]
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libstd/path.rs:680             _ => Some(Component::Normal(unsafe { u8_slice_as_os_str(comp) }))
                                                                                                   ^
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-nopt-t/build/src/libstd/path.rs:680:13: 680:14 help: pass `--explain E0001` to see a detailed explanation
error: aborting due to previous error
make: *** [x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/stamp.std] Error 101
program finished with exit code 2

[pythonesque]

I was just about to report this (and suggest the same solution). Is this still being merged?

[huonw]

I intend to get it merged, once I can debug the (reproducable) crash... absolutely no idea what is going on with it yet. 😢

[oli-obk]

I think I found it: range_inclusive (https://github.com/rust-lang/rust/blob/master/src/libcore/iter.rs#L2799-2808) won't work anymore due to the self.range.start == self.range.end check. This will cause https://github.com/rust-lang/rust/blob/master/src/librustc/middle/check_match.rs#L605 to produce 0 elements for range_inclusive(0, 0)

[alexcrichton]

Unfortunately I think this is backwards incompatible as-is, so I'm going to close this in the same manner as #26390

[pythonesque]

This is really, really, really unfortunate. The inability of the existing implementation to optimize properly is kind of absurd. We should at least have something in the documentation indicating this until the codegen is fixed.

[dotdash]

The original test case that led to this PR optimizes well now. Are there
other cases that still require this change to optimize well?
Am 19.06.2015 19:14 schrieb "Joshua Yanovski" [email protected]:

This is really, really, really unfortunate. The inability of the existing
implementation to optimize properly is kind of absurd. We should at least
have something in the documentation indicating this until the codegen is
fixed.

—
Reply to this email directly or view it on GitHub
#24705 (comment).