isvalid: return false out of bounds instead of throwing

also: `next` can assume that incoming indices are valid

Author: StefanKarpinski

base/strings/basic.jl

@@ -87,14 +87,13 @@ See also: [`ncodeunits`](@ref), [`checkbounds`](@ref)
 """
     isvalid(s::AbstractString, i::Integer) -> Bool
 
-Predicate indicating whether the given index is the start of the encoding of
-a character in `s` or not. If `isvalid(s, i)` is true then `s[i]` will return
-the character whose encoding starts at that index, if it's false, then `s[i]`
-will raise an invalid index error. Behavior of `next(s, i)` is similar except
-that the character is returned along with the index of the following character.
-In order for `isvalid(s, i)` to be an O(1) function, the encoding of `s` must
-be [self-synchronizing](https://en.wikipedia.org/wiki/Self-synchronizing_code);
-this is a basic assumption of Julia's generic string support.
+Predicate indicating whether the given index is the start of the encoding of a
+character in `s` or not. If `isvalid(s, i)` is true then `s[i]` will return the
+character whose encoding starts at that index, if it's false, then `s[i]` will
+raise an invalid index error or a bounds error depending on if `i` is in bounds.
+In order for `isvalid(s, i)` to be an O(1) function, the encoding of `s` must be
+[self-synchronizing](https://en.wikipedia.org/wiki/Self-synchronizing_code) this
+is a basic assumption of Julia's generic string support.
 
 See also: [`getindex`](@ref), [`next`](@ref), [`thisind`](@ref),
 [`nextind`](@ref), [`prevind`](@ref), [`length`](@ref)
@@ -128,8 +127,8 @@ Stacktrace:
 Return a tuple of the character in `s` at index `i` with the index of the start
 of the following character in `s`. This is the key method that allows strings to
 be iterated, yielding a sequences of characters. If `i` is out of bounds in `s`
-then a bounds error is raised; if `i` is not a valid character index in `s` then
-a Unicode index error is raised.
+then a bounds error is raised. The `next` function, as part of the iteration
+protocoal may assume that `i` is the start of a character in `s`.
 
 See also: [`getindex`](@ref), [`start`](@ref), [`done`](@ref),
 [`checkbounds`](@ref)
@@ -145,7 +144,11 @@ eltype(::Type{<:AbstractString}) = Char
 sizeof(s::AbstractString) = ncodeunits(s) * sizeof(codeunit(s))
 endof(s::AbstractString) = thisind(s, ncodeunits(s))
 
-getindex(s::AbstractString, i::Integer) = next(s, i)[1]
+function getindex(s::AbstractString, i::Integer)
+    @boundscheck checkbounds(s, i)
+    @inbounds return isvalid(s, i) ? next(s, i)[1] : string_index_err(s, i)
+end
+
 getindex(s::AbstractString, i::Colon) = s
 # TODO: handle other ranges with stride ±1 specially?
 # TODO: add more @propagate_inbounds annotations?

base/strings/search.jl

@@ -5,9 +5,7 @@ function search(s::String, c::Char, i::Integer = 1)
         i == sizeof(s) + 1 && return 0
         throw(BoundsError(s, i))
     end
-    @inbounds if is_valid_continuation(codeunit(s,i))
-        string_index_err(s, i)
-    end
+    @inbounds isvalid(s, i) || string_index_err(s, i)
     c ≤ '\x7f' && return search(s, c % UInt8, i)
     while true
         i = search(s, first_utf8_byte(c), i)
@@ -94,13 +92,10 @@ julia> search("JuliaLang","Julia")
 ```
 """
 function search(s::AbstractString, c::Chars, i::Integer)
-    if isempty(c)
-        return 1 <= i <= nextind(s,endof(s)) ? i :
-               throw(BoundsError(s, i))
-    end
-    if i < 1 || i > nextind(s,endof(s))
-        throw(BoundsError(s, i))
-    end
+    z = ncodeunits(s) + 1
+    isempty(c) && return 1 ≤ i ≤ z ? i : throw(BoundsError(s, i))
+    1 ≤ i ≤ z || throw(BoundsError(s, i))
+    @inbounds i == z || isvalid(s, i) || string_index_err(s, i)
     while !done(s,i)
         d, j = next(s,i)
         if d in c

base/strings/string.jl

@@ -157,11 +157,8 @@ is_valid_continuation(c) = c & 0xc0 == 0x80
     return next_continued(s, i, u)
 end
 
-@noinline function next_continued(s::String, i::Int, u::UInt32)
-    if u < 0xc0000000
-        isvalid(s, i) && (i += 1; @goto ret)
-        string_index_err(s, i)
-    end
+function next_continued(s::String, i::Int, u::UInt32)
+    u < 0xc0000000 && (i += 1; @goto ret)
     n = ncodeunits(s)
     # first continuation byte
     (i += 1) > n && @goto ret
@@ -281,11 +278,7 @@ first_utf8_byte(c::Char) = (reinterpret(UInt32, c) >> 24) % UInt8
 
 ## overload methods for efficiency ##
 
-function isvalid(s::String, i::Int)
-    @boundscheck checkbounds(s, i)
-    return thisind(s, i) == i
-end
-isvalid(s::String, i::Integer) = isvalid(s, Int(i))
+isvalid(s::String, i::Int) = checkbounds(Bool, s, i) && thisind(s, i) == i
 
 ## optimized concatenation, reverse, repeat ##
 

base/strings/substring.jl

@@ -73,8 +73,9 @@ function getindex(s::SubString, i::Integer)
 end
 
 function isvalid(s::SubString, i::Integer)
-    @boundscheck checkbounds(s, i)
-    @inbounds return isvalid(s.string, s.offset + i)
+    ib = true
+    @boundscheck ib = checkbounds(Bool, s, i)
+    @inbounds return ib && isvalid(s.string, s.offset + i)
 end
 
 thisind(s::SubString, i::Integer) = thisind(s.string, s.offset + i) - s.offset

test/strings/types.jl

@@ -169,12 +169,7 @@ let s = "lorem ipsum", sdict = Dict(
     for (ss, s) in sdict
         @test ncodeunits(ss) == ncodeunits(s)
         for i in -2:13
-            if 1 ≤ i ≤ ncodeunits(ss)
-                @test isvalid(ss, i) == isvalid(s, i)
-            else
-                @test_throws BoundsError isvalid(ss, i)
-                @test_throws BoundsError isvalid(s, i)
-            end
+            @test isvalid(ss, i) == isvalid(s, i)
         end
         for i in 1:ncodeunits(ss), j = i-1:ncodeunits(ss)
             @test length(ss, i, j) == length(s, i, j)