-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Description
In commit 7489a31, a new constructor for SubString
was added:
function SubString{T}(s::T, i::Int, j::Int, ::Val{:noshift}) where T<:AbstractString
@boundscheck if !(i == j == 0)
si, sj = i + 1, prevind(s, j + i + 1)
@inbounds isvalid(s, si) || string_index_err(s, si)
@inbounds isvalid(s, sj) || string_index_err(s, sj)
end
new(s, i, j)
end
This is undocumented, but it's a method of the public SubString
, and also tested.
I 100% sympathize with the need for having a substring constructor without checks that compiles to a noop.
However, I don't think it's a good method:
- It makes it way too easy to accidentally create a
SubString{SubString{T}}
, which I don't think ought be encouraged (or allowed, but that ship has sailed) - It's not clear from the signature that
i
andj
refers to offset and length, not start and stop indices, as it does in the other methods. This is very confusing. - It's a little strange with the
Val{:noshift}
as an argument. - Another minor issue is that the bounds- and index checks causes the constructor to not inline, even when annotated with
@inbounds
. So, it needs to be used@inbounds @inline ...
, which nobody is going to remember
It might possibly be breaking to remove this method. It's undocumented, but a method of a public type.
The only usage in the wild according to JuliaHub code search is JuliaSyntaxHighlighting.jl.
The method was added in #49586 without mentioning it, possibly it was meant to be a private method. Cc. @tecosaur.
I suggest creating a new (public or exported) function which allows users to skip index and bounds computations. Possibly unsafe_substring(s::AbstractString, first_codeunit::Int, last_codeunit::Int)
.