Handle character ranges starting at '\x00' #8524
Conversation
Do not calculate start-1 unless needed; start-1 may not be well-defined.
|
May be some utility in adding a test for this |
|
Minor, but I would change it to an outer constructor and special case |
| @@ -62,7 +62,7 @@ immutable UnitRange{T<:Real} <: OrdinalRange{T,Int} | |||
| start::T | |||
| stop::T | |||
|
|
|||
| UnitRange(start, stop) = new(start, ifelse(stop >= start, stop, start-1)) | |||
| UnitRange(start, stop) = new(start, stop >= start ? stop : start-1) | |||
There was a problem hiding this comment.
I think start-1 should be convert(T, start-true) to avoid type instability and unnecessary checked conversion. This seems like it was an oversight in the previous code.
Also if we go this route and keep Char arithmetic checked, then we should check whether there's meaningful overhead to branching here instead of using ifelse. My guess is that there isn't, since the branch no longer interferes with inlining, in many cases the branch can be hoisted or eliminated, and in most remaining cases it should be predictable, but if there is meaningful overhead maybe we should keep using ifelse for Union(Signed, Unsigned). Or just implement coerce and use that.
|
The nearby class |
|
I'm going to fix this a bit differently. |
Do not calculate start-1 unless needed; start-1 may not be well-defined.