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

Improve support for ranges with nonstandard Integers #27302

Merged
merged 4 commits into from
Jun 22, 2018
Merged

Improve support for ranges with nonstandard Integers #27302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a573b17
Improve support for nonstandard ranges.
tkoolen May 29, 2018
8a83c31
Remove : method that promotes step.
tkoolen May 29, 2018
e0dc6f8
Add test for #26619, fix underflow bug, fix length computation.
tkoolen May 29, 2018
d748060
Clean up steprange_last.
tkoolen May 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions base/range.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@

(:)(start::T, stop::T) where {T} = (:)(start, oftype(stop-start, 1), stop)

# first promote start and stop, leaving step alone
# promote start and stop, leaving step alone
(:)(start::A, step, stop::C) where {A<:Real,C<:Real} =
(:)(convert(promote_type(A,C),start), step, convert(promote_type(A,C),stop))
(:)(start::T, step::Real, stop::T) where {T<:Real} = (:)(promote(start, step, stop)...)

# AbstractFloat specializations
(:)(a::T, b::T) where {T<:AbstractFloat} = (:)(a, T(1), b)
Expand Down Expand Up @@ -144,7 +143,7 @@ function steprange_last(start::T, step, stop) where T
last = steprange_last_empty(start, step, stop)
else
diff = stop - start
if T<:Signed && (diff > zero(diff)) != (stop > start)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree, this seems a little strange. I understand why you need to remove this — and I agree it's a good thing to do, but I think we want to preserve the effective behavior here.

Perhaps we should detected (un)signed-ness based the behavior of stop - start > zero(diff) && start - stop > zero(diff)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's probably a good idea to code this up differently. I was going step by step through the logic and I think it's OK as it is, but it's not immediately obvious.

Copy link
Contributor Author

@tkoolen tkoolen May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why the current implementation works (but is quite tough to read):

  • diff > zero(diff) is always true for unsigned types at https://github.com/JuliaLang/julia/pull/27302/files#diff-c5053a63cd3df77d9079e95f29f350adR146 (since the case stop == start is handled separately)
  • hence, (diff > zero(diff)) != (stop > start) reduces to stop < start (again, stop == start is already handled), so far so good.
  • we'll get to remain = -convert(T, unsigned(-diff) % step). unsigned(-diff) === stop - start. T is an unsigned integer, so converting to T is a no-op. Finally, the negation results in overflow (unless unsigned(-diff) % step == 0).
  • last is computed as stop - remain, so (unless remain == 0) we're overflowing again and arriving at the correct answer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh my that's extremely valuable. Can you insert that as a comment?

if (diff > zero(diff)) != (stop > start)
# handle overflowed subtraction with unsigned rem
if diff > zero(diff)
remain = -convert(T, unsigned(-diff) % step)
Expand Down Expand Up @@ -374,7 +373,7 @@ julia> step(range(2.5, stop=10.9, length=85))
```
"""
step(r::StepRange) = r.step
step(r::AbstractUnitRange{T}) where{T} = oneunit(T)
step(r::AbstractUnitRange{T}) where{T} = oneunit(T) - zero(T)
step(r::StepRangeLen{T}) where {T} = T(r.step)
step(r::LinRange) = (last(r)-first(r))/r.lendiv

Expand All @@ -388,8 +387,8 @@ function unsafe_length(r::StepRange)
isempty(r) ? zero(n) : n
end
length(r::StepRange) = unsafe_length(r)
unsafe_length(r::AbstractUnitRange) = Integer(last(r) - first(r) + 1)
unsafe_length(r::OneTo) = r.stop
unsafe_length(r::AbstractUnitRange) = Integer(last(r) - first(r) + step(r))
unsafe_length(r::OneTo) = Integer(r.stop - zero(r.stop))
length(r::AbstractUnitRange) = unsafe_length(r)
length(r::OneTo) = unsafe_length(r)
length(r::StepRangeLen) = r.len
Expand All @@ -401,8 +400,10 @@ function length(r::StepRange{T}) where T<:Union{Int,UInt,Int64,UInt64}
return checked_add(convert(T, div(unsigned(r.stop - r.start), r.step)), one(T))
elseif r.step < -1
return checked_add(convert(T, div(unsigned(r.start - r.stop), -r.step)), one(T))
elseif r.step > 0
return checked_add(div(checked_sub(r.stop, r.start), r.step), one(T))
else
checked_add(div(checked_sub(r.stop, r.start), r.step), one(T))
return checked_add(div(checked_sub(r.start, r.stop), -r.step), one(T))
end
end

Expand Down
77 changes: 77 additions & 0 deletions test/ranges.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,3 +1276,80 @@ end
@test step(x) == 0.0
@test x isa StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}
end

module NonStandardIntegerRangeTest

using Test

struct Position <: Integer
val::Int
end
Position(x::Position) = x # to resolve ambiguity with boot.jl:728

struct Displacement <: Integer
val::Int
end
Displacement(x::Displacement) = x # to resolve ambiguity with boot.jl:728

Base.:-(x::Displacement) = Displacement(-x.val)
Base.:-(x::Position, y::Position) = Displacement(x.val - y.val)
Base.:-(x::Position, y::Displacement) = Position(x.val - y.val)
Base.:-(x::Displacement, y::Displacement) = Displacement(x.val - y.val)
Base.:+(x::Position, y::Displacement) = Position(x.val + y.val)
Base.:+(x::Displacement, y::Displacement) = Displacement(x.val + y.val)
Base.:(<=)(x::Position, y::Position) = x.val <= y.val
Base.:(<)(x::Position, y::Position) = x.val < y.val
Base.:(<)(x::Displacement, y::Displacement) = x.val < y.val

# for StepRange computation:
Base.Unsigned(x::Displacement) = Unsigned(x.val)
Base.rem(x::Displacement, y::Displacement) = Displacement(rem(x.val, y.val))
Base.div(x::Displacement, y::Displacement) = Displacement(div(x.val, y.val))

# required for collect (summing lengths); alternatively, should unsafe_length return Int by default?
Base.promote_rule(::Type{Displacement}, ::Type{Int}) = Int
Base.convert(::Type{Int}, x::Displacement) = x.val

@testset "Ranges with nonstandard Integers" begin
for (start, stop) in [(2, 4), (3, 3), (3, -2)]
@test collect(Position(start) : Position(stop)) == Position.(start : stop)
end

for start in [3, 0, -2]
@test collect(Base.OneTo(Position(start))) == Position.(Base.OneTo(start))
end

for step in [-3, -2, -1, 1, 2, 3]
for start in [-1, 0, 2]
for stop in [start, start - 1, start + 2 * step, start + 2 * step + 1]
r1 = StepRange(Position(start), Displacement(step), Position(stop))
@test collect(r1) == Position.(start : step : stop)

r2 = Position(start) : Displacement(step) : Position(stop)
@test r1 === r2
end
end
end
end

end # module NonStandardIntegerRangeTest

@testset "Issue #26619" begin
@test length(UInt(100) : -1 : 1) === UInt(100)
@test collect(UInt(5) : -1 : 3) == [UInt(5), UInt(4), UInt(3)]

let r = UInt(5) : -2 : 2
@test r.start === UInt(5)
@test r.step === -2
@test r.stop === UInt(3)
@test collect(r) == [UInt(5), UInt(3)]
end

for step in [-3, -2, -1, 1, 2, 3]
for start in [0, 15]
for stop in [0, 15]
@test collect(UInt(start) : step : UInt(stop)) == start : step : stop
end
end
end
end