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

Implement range syntax #19858

Merged
merged 6 commits into from Dec 24, 2014

Conversation

Projects
None yet
8 participants
@nrc
Copy link
Member

nrc commented Dec 15, 2014

Closes #19794

r? @aturon for the first patch
r? @nikomatsakis for the rest

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 15, 2014

Yay! Delighted to see this, @nick29581

#[lang="full_range"]
pub struct FullRange;

/// A range which i bounded at both ends.

This comment has been minimized.

@sfackler

sfackler Dec 15, 2014

Member

"is". Might want to mention "half open" in the type docs as well.


return None;
}
}

This comment has been minimized.

@sfackler

sfackler Dec 15, 2014

Member

Might as well implement ExactSizeIterator as well.

impl<Idx: Clone + Countable> Iterator<Idx> for RangeFrom<Idx> {
#[inline]
fn next(&mut self) -> Option<Idx> {
// Deliberately overflow so we loop forever.

This comment has been minimized.

@sfackler

sfackler Dec 15, 2014

Member

Do we want to do this or stop at the maximum value?

This comment has been minimized.

@sfackler

sfackler Dec 15, 2014

Member

Now that I think about it, does it even make sense for this to implement Iterator?

This comment has been minimized.

@nrc

nrc Dec 15, 2014

Author Member

I had a bit of a discussion about this on IRC and the consensus was yes it makes sense (although basically only to use take on) and that looping forever was probably what you would expect, rather than stopping at the max int (it's also more efficient).

This comment has been minimized.

@aturon

aturon Dec 16, 2014

Member

Note, we've long had this iterator: http://static.rust-lang.org/doc/master/std/iter/fn.count.html

This notation will just serve to make it a bit more convenient. I'm hoping that we can drop many of the bare fns in std::iter in favor of this notation.

#[inline]
fn decrement(&mut self) { *self -= 1; }
#[inline]
fn difference(a: &$t, b: &$t) -> uint { (*a - *b) as uint }

This comment has been minimized.

@huonw

huonw Dec 15, 2014

Member

uint doesn't always work for u64/i64 on 32-bit platforms.

This comment has been minimized.

@nrc

nrc Dec 15, 2014

Author Member

I guess T should be u64 for u64/i64 and we require T=uint for to implement iterator. We need the snapshot for that, but hopefully it can happen before this lands

/// The difference between two countable objects.
/// Temporarily a uint, should be an associated type, but
// FIXME(#19391) needs a snapshot
fn difference(a: &Self, b: &Self) -> uint;

This comment has been minimized.

@huonw

huonw Dec 15, 2014

Member

Shouldn't this be signed?

This comment has been minimized.

@nrc

nrc Dec 15, 2014

Author Member

Urgh, yes, kinda, the problem is that then it can overflow. I think I should just comment better that this is the magnitude of the difference and that we assume a > b (I could debug_assert that)

}
}

impl<Idx: Copy> Copy for Range<Idx> {}

This comment has been minimized.

@huonw

huonw Dec 15, 2014

Member

Can these be derived?

This comment has been minimized.

@nrc

nrc Dec 15, 2014

Author Member

IDK - is deriving clever enough to add bounds on generics like this?

This comment has been minimized.

@sfackler
#[test]
fn test_range() {
let r = Range { start: 2u, end: 10 };
for (i, ri) in r.enumerate() {

This comment has been minimized.

@huonw

huonw Dec 15, 2014

Member

This test doesn't detect the iteration being shorter than expected (or not occur at all), could be something like

let mut r = Range { ... };
let mut i = 0;
for ri in r {
    assert!(...);
    assert!(...);
    i += 1;
}
assert_eq!(i, 8);
@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 15, 2014

@nick29581 left some comments -- though possibly on a stale commit? In any case, I'm basically fine with landing this as-is in terms of naming and placement of the Countable trait -- I have some thoughts but can move things around during stabilization.

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Dec 15, 2014

@aturon I like the Step idea, but I prefer that we use an associated type for the result of steps_left, otherwise you get into overflow territory. I think we can use an equality constraint on the associated type <T=uint> to make this work for the size hint (i.e., you only get an Iterator implementation when it is safe to give a size hint, with better trait matching in the future, we could do better and always get an Iterator impl, but only give a size hint if it is safe).

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 15, 2014

@nick29581 What about making it return Option<uint>? Its sole purpose is for the size hint, right?

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Dec 15, 2014

@aturon it is for now, yes (if the trait is meant to be more generally useful, then I guess it should handle more here, but perhaps we should ere on the side of YAGNI here.). Does size_hint need to be fast? I.e., is it worth trying to avoid the branch? If not, then Option seems fine for now.

@nrc nrc assigned nikomatsakis and unassigned aturon Dec 15, 2014

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 15, 2014

Usually size_hint is called ahead of fully iterating, so the branch
shouldn't matter much -- but in any case you should be able to use
#[inline].

I also agree that YAGNI seems best here. We recently axed a lot of the
numerics traits to punt general numerics treatments to the Cargoverse --
partly because there are so many ways to do it. Staying focused on the
main use case of iterating over range syntax seems best.

On Mon, Dec 15, 2014 at 3:48 PM, Nick Cameron notifications@github.com
wrote:

@aturon https://github.com/aturon it is for now, yes (if the trait is
meant to be more generally useful, then I guess it should handle more here,
but perhaps we should ere on the side of YAGNI here.). Does size_hint need
to be fast? I.e., is it worth trying to avoid the branch? If not, then
Option seems fine for now.


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

@sinistersnare

This comment has been minimized.

Copy link
Contributor

sinistersnare commented Dec 16, 2014

Is something like the following in this PR? Just a thought I had a few minutes ago.

for i in (0..) {
    println!("infinite loop! i: {}", i);
}

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Dec 16, 2014

@sinistersnare yes, that will work, you don't even need the brackets around the range

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 16, 2014

Exciting.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 16, 2014

So this looks reasonable, though I'm wondering if we can just desugar these expressions in the front-end and save ourselves a lot of weird bugs down the line. This seems like a classic case.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 16, 2014

r+ modulo comments and in particular the question about using a common supertype, though perhaps we can discuss whether to desugar etc on IRC

@nrc nrc force-pushed the nrc:ranges branch from 97a4ffd to ef4a1f4 Dec 18, 2014

bors added a commit that referenced this pull request Dec 18, 2014

/// Change self to the previous object.
fn step_back(&mut self);
/// The steps_between two step objects.
/// a should always be less than b, so the result should never be negtive.

This comment has been minimized.

@quantheory

quantheory Dec 20, 2014

Contributor

s/negtive/negative/

let _ = 0u..4+4-3;
let _ = 0..foo();

// Test we can use tow different types with a common supertype.

This comment has been minimized.

@quantheory

quantheory Dec 20, 2014

Contributor

s/tow/two/

@nrc nrc referenced this pull request Dec 23, 2014

Merged

Remove ExprSlice #20160

@nrc nrc force-pushed the nrc:ranges branch from ef4a1f4 to 4d5d24d Dec 23, 2014

bors added a commit that referenced this pull request Dec 23, 2014

@nrc nrc force-pushed the nrc:ranges branch from 4d5d24d to e82215d Dec 23, 2014

@nrc

This comment has been minimized.

Copy link
Owner Author

nrc commented on e82215d Dec 23, 2014

r=aturon,nikomatsakis

This comment has been minimized.

Copy link
Owner Author

nrc replied Dec 24, 2014

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on e82215d Dec 24, 2014

saw approval from aturon
at nrc@e82215d

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

merging nick29581/rust/ranges = e82215d into auto

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

status: {"merge_sha": "0e4d1a02deb0e4d4c3c36516713f2d97b16d5f00"}

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

nick29581/rust/ranges = e82215d merged ok, testing candidate = 0e4d1a0

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

status: {"merge_sha": "e64a8193b02ce72ef183274994a25eae281cb89c"}

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

nick29581/rust/ranges = e82215d merged ok, testing candidate = e64a819

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

fast-forwarding master to auto = e64a819

This comment has been minimized.

Copy link
Contributor

bors replied Dec 24, 2014

fast-forwarding master to auto = e64a819

bors added a commit that referenced this pull request Dec 24, 2014

bors added a commit that referenced this pull request Dec 24, 2014

auto merge of #19858 : nick29581/rust/ranges, r=aturon
Closes #19794

r? @aturon for the first patch
r? @nikomatsakis for the rest

@bors bors merged commit e82215d into rust-lang:master Dec 24, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default all tests passed

bors added a commit that referenced this pull request Dec 26, 2014

auto merge of #20160 : nick29581/rust/ranges2, r=nikomatsakis
The first six commits are from an earlier PR (#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)

bors added a commit that referenced this pull request Dec 28, 2014

auto merge of #20160 : nick29581/rust/ranges2, r=nikomatsakis
The first six commits are from an earlier PR (#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)

bors added a commit that referenced this pull request Dec 28, 2014

auto merge of #20160 : nick29581/rust/ranges2, r=nikomatsakis
The first six commits are from an earlier PR (#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)

bors added a commit that referenced this pull request Dec 28, 2014

auto merge of #20160 : nick29581/rust/ranges2, r=nikomatsakis
The first six commits are from an earlier PR (#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)

bors added a commit that referenced this pull request Dec 30, 2014

auto merge of #20160 : nick29581/rust/ranges2, r=nikomatsakis
The first six commits are from an earlier PR (#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 30, 2014

rollup merge of rust-lang#20160: nick29581/ranges2
The first six commits are from an earlier PR (rust-lang#19858) and have already been reviewed. This PR makes an awful hack in the compiler to accommodate slices both natively and in the index a range form. After a snapshot we can hopefully add the new Index impls and then we can remove these awful hacks.

r? @nikomatsakis (or anyone who knows the compiler, really)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.