Fix range construction and length

[timholy]

We had a surprising number of problems with ranges, for example:

julia> length(N0f8(0):eps(N0f8):N0f8(1))
0x00

julia> length(N0f8(0):N0f8(1))
ERROR: InexactError: Int64(0.996078431372549)
Stacktrace:
 [1] Int64 at ./float.jl:710 [inlined]
 [2] Integer at ./boot.jl:741 [inlined]
 [3] convert at ./number.jl:7 [inlined]
 [4] Integer at /home/tim/.julia/dev/FixedPointNumbers/src/normed.jl:226 [inlined]
 [5] unsafe_length at ./range.jl:516 [inlined]
 [6] length(::UnitRange{Normed{UInt8,8}}) at ./range.jl:518
 [7] top-level scope at none:0

[kimikage]

I think this is very nice. However, this (semantically) conflicts with PR #158. Which one to merge first?

[kimikage]

You don't have to follow my draft naming guidelines, but I hope you update the guidelines to suit your preferences. #139 (comment)

Naming is not just a matter of taste; it is important for understanding the test codes, i.e. the expected behavior.

[kimikage]

I think the return type of length is related to the issue #154. I'm going to change the return type of Integer to the raw type T.
I means that perhaps this could be a little more commonized.

Is there a problem with length returning an Unsigned number?

Edit:
Of course, even if we (internally) use Unsigned, typemin(X):eps(X):typemax(X) requires the widening. In other words, that is the only exception, so its specialization may be worth considering.
FWIW, as a matter of fact, it throws an OverflowError in this case of UInt64.

julia> length(typemin(UInt64):typemax(UInt64))
ERROR: OverflowError: 18446744073709551615 + 1 overflowed for type UInt64

[kimikage]

BTW, why did you decide to specialize StepRange{X,Y} where {X <: FixedPoint, Y <: FixedPoint}?
It can definitely be optimized with StepRange{X, X}. On the other hand, I can hardly find any advantage in restricting Y to FixedPoint.

[timholy]

Is there a problem with length returning an Unsigned number?

Not a problem, but array indexing in Julia is specialized for Int. Hence length should return Int when possible.

julia> length(0x00:0xff)
256

[kimikage]

My idea is that the internal function (something like unsafe_length) should return an Unsigned number with the same bit width as raw type T, and the return values should be promoted to Int in Base.length. However, I haven't tried it, so I don't know whether it will be really simple.

Edit:
Of course, the improvement should be done after fixing the bugs. However, I want to clarify the specification of the return type now.

[codecov-io]

Codecov Report

Merging #163 into master will decrease coverage by 0.18%.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##           master    #163      +/-   ##
=========================================
- Coverage   88.08%   87.9%   -0.19%     
=========================================
  Files           5       5              
  Lines         361     372      +11     
=========================================
+ Hits          318     327       +9     
- Misses         43      45       +2

Impacted Files	Coverage Δ
src/utilities.jl	73.68% <ø> (ø)	⬆️
src/fixed.jl	87.8% <100%> (+0.46%)	⬆️
src/normed.jl	89.65% <100%> (+0.12%)	⬆️
src/FixedPointNumbers.jl	84.21% <66.66%> (-1.51%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 402e637...991647c. Read the comment docs.

[timholy]

I'd agree except that length(0x00:0xff) ==256, which is not expressible as a UInt8. Consequently even the internal function would need either a checked or promoting method. At that point I start thinking that just implementing length and unsafe_length (both of which are required for proper Base support) seems enough.

[kimikage]

TBH, I don't understand why we have to implement unsafe_length. Even if it is necessary, the essence does not change. The caller of the internal function just changes from length to unsafe_length.

In any case, what I care about is the return type of length. Once the specification is in place, it is clear whether the implementation should be modified in the future.

[kimikage]

I think Base.length(r::AbstractUnitRange{N}) is broken.
That being said, I did not notice the problem with length. This is a big step forward.

[timholy]

I don't understand why we have to implement unsafe_length

We don't unless we think it's useful for optimizing bounds-checking (do a $ grep -R unsafe_length * from Julia's base/ directory). Given that I don't really expect anything bigger than 16-bit ranges to be used in practice, and these are covered even on 32-bit machines by ShorterThanInt, I have no objections if you'd rather remove this.

In any case, what I care about is the return type of length. Once the specification is in place, it is clear whether the implementation should be modified in the future.

I think the specification is: return Int for T (the rawtype) with bitwidth(T) <= bitwidth(Int), and return T for anything bigger.

[timholy]

I eliminated the unsafe_length methods altogether. It only affects performance of ranges constructed with Int/UInt, which is not a case I care about in practice, and it's definitely safer to do the checking if performance isn't a problem. So, a fine suggestion.

[kimikage]

I'm sorry for pushing my idea on you, but it looks good to me.:+1:

[timholy]

Thanks for the review!