Specialisation of Iterator methods for Range by nagisa · Pull Request #42534 · rust-lang/rust

nagisa · 2017-06-08T13:27:45Z

This is based onto #42456. Should not be merged before that lands.

This also removes the StepBy deprecated in previous cycle.

This commit is a result of the libs team's discussion of rust-lang#33417 and how it affects integral types. The conclusion was that the motivation for converting integral types, working in a cross-platform code that uses platform-specific integer types, was different enough from the intent of `TryFrom` that it doesn't make sense to unify the paths. As a result this is a proposal for the alternative version of the API which purely works with integral types. An unstable `Cast` trait is added as the implementation detail of this API, and otherwise with this you should be able to call `i32::cast(0u8)` at will. The intention is then to call this in platform-specific contexts like: // Convert from C to Rust let a: c_int = ...; let b: u32 = u32::cast(a)?; // Convert from Rust to C let a: u32 = ...; let b: c_int = <c_int>::cast(a)?; Everything here is unstable for now, but the intention is that this will stabilize sooner than `TryFrom`.

rust-highfive · 2017-06-08T13:27:58Z

r? @BurntSushi

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

nagisa · 2017-06-08T17:42:58Z

For anybody triaging this: I will not bother fixing the test failures until the cast PR lands. This is mostly a waiting-on-review but also waiting-on-other-pr.

scottmcm · 2017-06-09T00:15:32Z

I believe this also Fixes #41477 (by deleting it)

scottmcm · 2017-06-09T05:37:25Z

+
+ #[inline]
+ fn count(self) -> usize {
+ usize::MAX


Should this, like last, also panic? Returning a value here is neither of the behaviours (panic or diverge) possible from this today.

As the Iterator implementation for RangeFrom in fundamentally broken at the moment (#25708), I don't think it's a good idea to override count, nth and last just yet. At least the default impls match what happens when you repeatedly call .next().

@scottmcm the documentation of Iterator::count says:
The method does no guarding against overflows, so counting elements of an iterator with more than usize::MAX elements either produces the wrong result or panics. If debug assertions are enabled, a panic is guaranteed.
so yes, it should panic, as you say.

@ollie27 I’m fairly confident that nth maintains the same behaviour as the current next by being ever-so-slightly broken around the last value.

Well with debug assertions disabled (0u8..).nth(1000) returns Some(232) and by the looks of it your version will always panic. Ideally it would return None. I guess adding panics in cases where the behaviour isn't properly defined isn't too bad but I'd like to see this implementation properly fixed.

scottmcm · 2017-06-09T05:38:54Z

 #![allow(deprecated)]

 assert_eq!((0..20).step_by(5).collect::<Vec<isize>>(), [0, 5, 10, 15]);
- assert_eq!((20..0).step_by(-5).collect::<Vec<isize>>(), [20, 15, 10, 5]);


Would it be worth rephrasing these in terms of rev? Like, for this one,
assert_eq!((1..21).rev().step_by(5).collect::<Vec<isize>>(),[20,15,10,5]);

leonardo-m · 2017-06-09T22:17:26Z

Is the new StepBy on (x..y) intervals efficient? Because this is the most common use case, and it's a quite common one.

nagisa · 2017-06-09T22:30:13Z

#![feature(iterator_step_by)]pubfnfoo(x:::std::ops::Range<i32>) -> i32{letmut sum = 0;for a in x.step_by(3){
sum += a;}return sum;}

results in this being generated:

Details

	.text
.file	"foo.cgu-0.rs"
.section	.text._ZN3foo3foo17h6610c3c3ce7ddf2aE,"ax",@progbits
.globl	_ZN3foo3foo17h6610c3c3ce7ddf2aE
.p2align	4, 0x90
.type	_ZN3foo3foo17h6610c3c3ce7ddf2aE,@function
_ZN3foo3foo17h6610c3c3ce7ddf2aE:
.cfi_startproc
pushq	%rbp
.Lcfi0:
.cfi_def_cfa_offset 16
pushq	%r15
.Lcfi1:
.cfi_def_cfa_offset 24
pushq	%r14
.Lcfi2:
.cfi_def_cfa_offset 32
pushq	%rbx
.Lcfi3:
.cfi_def_cfa_offset 40
pushq	%rax
.Lcfi4:
.cfi_def_cfa_offset 48
.Lcfi5:
.cfi_offset %rbx, -40
.Lcfi6:
.cfi_offset %r14, -32
.Lcfi7:
.cfi_offset %r15, -24
.Lcfi8:
.cfi_offset %rbp, -16
movq	%rdi, %rbx
movq	%rbx, %r14
shrq	$32, %r14
xorl	%r15d, %r15d
xorl	%eax, %eax
xorl	%ebp, %ebp
testb	$1, %al
jne	.LBB0_3
jmp	.LBB0_2
.p2align	4, 0x90
.LBB0_9:
shrq	$32, %rcx
addl	%ebp, %ecx
movb	$1, %al
movl	%ecx, %ebp
testb	$1, %al
je	.LBB0_2
.LBB0_3:
movl	$2, %edi
callq	_ZN58_$LT$i32$u20$as$u20$core..num..cast..Cast$LT$usize$GT$$GT$4cast17hc851b0f2c71d4b6fE@PLT
testl	%eax, %eax
je	.LBB0_5
xorl	%ecx, %ecx
xorl	%edx, %edx
jmp	.LBB0_6
.p2align	4, 0x90
.LBB0_2:
movq	%rbx, %rax
shlq	$32, %rax
xorl	%ecx, %ecx
cmpl	%r14d, %ebx
setl	%cl
cmovgeq	%r15, %rax
addl	%ecx, %ebx
orq	%rax, %rcx
testl	%ecx, %ecx
jne	.LBB0_9
jmp	.LBB0_8
.p2align	4, 0x90
.LBB0_5:
shrq	$32, %rax
addl	%ebx, %eax
seto	%sil
movq	%rax, %rdx
shlq	$32, %rdx
xorb	$1, %sil
cmpl	%eax, %r14d
setg	%cl
incl	%eax
andb	%sil, %cl
cmovel	%ebx, %eax
movzbl	%cl, %ecx
cmoveq	%r15, %rdx
movl	%eax, %ebx
.LBB0_6:
orq	%rdx, %rcx
testl	%ecx, %ecx
jne	.LBB0_9
.LBB0_8:
movl	%ebp, %eax
addq	$8, %rsp
popq	%rbx
popq	%r14
popq	%r15
popq	%rbp
retq
.Lfunc_end0:
.size	_ZN3foo3foo17h6610c3c3ce7ddf2aE, .Lfunc_end0-_ZN3foo3foo17h6610c3c3ce7ddf2aE
.cfi_endproc

so it does use the new nth.

leonardo-m · 2017-06-09T22:34:15Z

@nagisa your code gives me:

error: use of unstable library feature 'step_by': recent addition (see issue #27741)
--> ...\test.rs:4:16
|
4 | for a in x.step_by(3) {
| ^^^^^^^
|
= help: add #![feature(step_by)] to the crate attributes to enable

Compiling with the latest Nightly (given by rustup show):

Default host: x86_64-pc-windows-gnu
nightly-x86_64-pc-windows-gnu (default)
rustc 1.19.0-nightly (148e91714 2017-06-08)

nagisa · 2017-06-09T22:40:29Z

@leonardo-m because it would still be using the DeprecatedStepBy on nightly. Replace x.step_by with Iterator::step_by(&mut x, 3) or some such.

leonardo-m · 2017-06-09T22:49:14Z

Thank you @nagisa, adding a "mut" it works. I have this test code:

#![feature(iterator_step_by, step_by)]
#[inline(never)]
pub fn first_foo(mut a: i32, b: i32) -> i32 {
let mut tot = 0;
while a < b {
tot += a;
a += 3;
}
tot
}
#[inline(never)]
pub fn second_foo(x: std::ops::Range<i32>) -> i32 {
let mut tot = 0;
#[allow(deprecated)]
for a in x.step_by(3) {
tot += a;
}
tot
}
#[inline(never)]
pub fn third_foo(mut x: std::ops::Range<i32>) -> i32 {
let mut tot = 0;
for a in Iterator::step_by(&mut x, 3) {
tot += a;
}
tot
}
fn main() {
println!("{}", first_foo(0, 1000));
println!("{}", first_foo(0, 100000));
println!("{}", second_foo(0 .. 1000));
println!("{}", second_foo(0 .. 100000));
println!("{}", third_foo(0 .. 1000));
println!("{}", third_foo(0 .. 100000));
}

Compiling with:
rustc -O --emit asm test.rs

It gives:

_ZN5test29first_foo17hc5fe5ef1ed09d5a4E:
testl %ecx, %ecx
jle .LBB0_1
xorl %edx, %edx
xorl %eax, %eax
.p2align 4, 0x90
.LBB0_4:
addl %edx, %eax
addl $3, %edx
cmpl %ecx, %edx
jl .LBB0_4
jmp .LBB0_2
.LBB0_1:
xorl %eax, %eax
.LBB0_2:
retq
_ZN5test210second_foo17h5b0eb3168418bdd1E:
movq %rcx, %rdx
shrq $32, %rdx
xorl %eax, %eax
cmpl %edx, %ecx
jge .LBB1_3
xorl %eax, %eax
.p2align 4, 0x90
.LBB1_2:
addl %ecx, %eax
addl $3, %ecx
cmovol %edx, %ecx ; what is this for?
cmpl %edx, %ecx
jl .LBB1_2
.LBB1_3:
retq
_ZN5test29third_foo17h179cbb320b2d4169E:
movq %rcx, %r9
shrq $32, %r9
xorl %r8d, %r8d
xorl %r10d, %r10d
xorl %eax, %eax
testb $1, %r10b
je .LBB2_11
jmp .LBB2_2
.p2align 4, 0x90
.LBB2_7:
shrq $32, %rdx
addl %eax, %edx
movb $1, %r10b
movl %edx, %eax
testb $1, %r10b
je .LBB2_11
.LBB2_2:
cmpl %r9d, %ecx
jge .LBB2_5
leal 1(%rcx), %edx
cmpl %r9d, %edx
jge .LBB2_4
leal 2(%rcx), %edx
cmpl %r9d, %edx
jge .LBB2_9
addl $3, %ecx
shlq $32, %rdx
movl $1, %r10d
jmp .LBB2_6
.p2align 4, 0x90
.LBB2_11:
movq %rcx, %r10
shlq $32, %r10
xorl %edx, %edx
cmpl %r9d, %ecx
setl %dl
cmovgeq %r8, %r10
addl %edx, %ecx
jmp .LBB2_6
.p2align 4, 0x90
.LBB2_4:
movl %edx, %ecx
jmp .LBB2_5
.LBB2_9:
movl %edx, %ecx
.p2align 4, 0x90
.LBB2_5:
xorl %edx, %edx
xorl %r10d, %r10d
.LBB2_6:
orq %r10, %rdx
testl %edx, %edx
jne .LBB2_7
retq

The new step_by isn't acceptable, I think.

bors · 2017-06-14T02:57:15Z

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

arielb1 · 2017-06-20T20:31:48Z

Friendly ping to keep this on your radar @nagisa.

arielb1 · 2017-06-27T20:52:22Z

🕸 Closing PR due to inactivity to keep our backlog clean. Feel free to reopen. 🕸

leonardo-m · 2017-06-30T22:27:11Z

I use step_by often in my code, and now my code has many:

warning: use of deprecated item: replaced by Iterator::step_by

So the situation isn't in a stable state.

alexcrichtonand others added 4 commits June 8, 2017 16:23

Remove the deprecated StepBy
748f826

Specialisations for Range::{last,nth,count}
7b7dcc5

Specialise the Range::last
ba134df

rust-highfive assigned BurntSushi Jun 8, 2017

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2017

nagisa force-pushed the range-iter branch from af9e03d to 13dfda5Compare June 8, 2017 15:00

scottmcm reviewed Jun 9, 2017
View reviewed changes

scottmcm mentioned this pull request Jun 9, 2017
Tracking issue for step_trait stabilization #42168
Open
5 tasks

nagisa force-pushed the range-iter branch from bf1f18d to 1a65153Compare June 9, 2017 11:42

nagisa added 2 commits June 9, 2017 15:25

Fix the <RangeFrom<A> as Iterator>::count
667734e

More tests
8b80368

nagisa force-pushed the range-iter branch from 1a65153 to 8b80368Compare June 9, 2017 12:26

Mark-Simulacrum 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 Jun 25, 2017

arielb1 closed this Jun 27, 2017

leonardo-m mentioned this pull request Jul 5, 2017
The new step_by is too much slow #43064
Closed

Uh oh!

Uh oh!

Conversation

nagisa commented Jun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jun 8, 2017

Uh oh!

nagisa commented Jun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm commented Jun 9, 2017

Uh oh!

scottmcmJun 9, 2017

Choose a reason for hiding this comment

Uh oh!

ollie27Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

nagisaJun 9, 2017

Choose a reason for hiding this comment

Uh oh!

nagisaJun 9, 2017

Choose a reason for hiding this comment

Uh oh!

ollie27Jun 9, 2017

Choose a reason for hiding this comment

Uh oh!

scottmcmJun 9, 2017

Choose a reason for hiding this comment

Uh oh!

leonardo-m commented Jun 9, 2017

Uh oh!

nagisa commented Jun 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardo-m commented Jun 9, 2017

Uh oh!

nagisa commented Jun 9, 2017

Uh oh!

leonardo-m commented Jun 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jun 14, 2017

Uh oh!

arielb1 commented Jun 20, 2017

Uh oh!

arielb1 commented Jun 27, 2017

Uh oh!

leonardo-m commented Jun 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

nagisa commented Jun 8, 2017•
edited
Loading

nagisa commented Jun 8, 2017•
edited
Loading

nagisa commented Jun 9, 2017•
edited
Loading

leonardo-m commented Jun 9, 2017•
edited
Loading