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

Fix for last{T}(t::Range{T}) #2734

Merged
merged 1 commit into from
Apr 3, 2013
Merged

Conversation

kmsquire
Copy link
Member

@kmsquire kmsquire commented Apr 2, 2013

  • Because of automatic promotion of small integer types to
    Int/Uint, last(t::Range) was a different type than start(t).
    This commit fixes that.

* Because of automatic promotion of small integer types to
  Int/Uint, last(t::Range) was a different type than start(t).
  This commit fixes that.
@StefanKarpinski
Copy link
Member

Seems sane to me. @JeffBezanson?

@kmsquire
Copy link
Member Author

kmsquire commented Apr 2, 2013

Slightly more info: what prompted this was

julia> uint16(1):uint16(10)
0x0001:0x000000000000000a

which, after the commit, becomes the more expected

julia> uint16(1):uint16(10)
0x0001:0x000a

@JeffBezanson
Copy link
Member

The problem is a bit more extensive than this. Everything next returns is computed, so as an iterator Uint64s are generated, but getindex returns Uint16s.

I'm starting to question our integer arithmetic again; it has caused a few surprises, and has the potential to generate type-unstable loops even when all inputs are the same type.

@StefanKarpinski
Copy link
Member

That's unfortunate, but I have to say, it's been a long time since I've encountered an unpleasant surprise and I tend to find that it just does what I want, which is nice. We could also just implement henchmen unrolling.

@JeffBezanson
Copy link
Member

We need henchmen unrolling anyway, so the problem is more the first part, that you need oftype everywhere in the range code, or next and getindex give different types.

@StefanKarpinski
Copy link
Member

I think something much deeper is wrong with the range stuff and was working on a fix, but doing anything with ranges is brutal because it's so deep in the language and if anything goes wrong you're toast. In any case, I think it's safe to merge this. Was just pinging you since you did the most recent major surgery on ranges.

@JeffBezanson
Copy link
Member

Is there something else to fix in ranges besides the floating point accuracy thing?

@StefanKarpinski
Copy link
Member

The current state just feels pretty ugly and changing it to accommodate the floating-point accuracy thing is kind of hard given the way it currently works.

@JeffBezanson
Copy link
Member

Well at least now we can remove OrdinalRange if we want.

@StefanKarpinski
Copy link
Member

That would certainly be an improvement.

JeffBezanson added a commit that referenced this pull request Apr 3, 2013
@JeffBezanson JeffBezanson merged commit e4f831d into JuliaLang:master Apr 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants