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.

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

Allow AbstractUnitRange for string getindex #41807

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Aug 6, 2021

Several methods could be generalized to work with AbstractUnitRange rather than just UnitRange. There are now several AbstractUnitRange subtypes:

julia> subtypes(AbstractUnitRange)
4-element Vector{Any}:
 Base.IdentityUnitRange
 Base.OneTo
 Base.Slice
 UnitRange

This pull request allows AbstractUnitRange for getindex on a String.

There seem to be some ambiguous method errors associated with the current change.

@vtjnash
Copy link
Member

vtjnash commented Aug 16, 2021

There seem to be some ambiguous method errors

Looks like another method signature update, that was also simply too strict previously, is needed here

getindex(s::AbstractString, r::UnitRange{<:Integer}) = SubString(s, r)

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Aug 17, 2021
@mkitti mkitti marked this pull request as ready for review August 17, 2021 00:50
@vtjnash vtjnash merged commit 0771939 into JuliaLang:master Aug 18, 2021
@timholy
Copy link
Member

timholy commented Aug 19, 2021

What does s[OffsetArray(1:3, -1:1)] do? I guess that's technically not a range, but nothing prevents me from writing one whose values start at 1 but whose indices start at something that String can't support. E.g,

julia> r = OffsetArrays.IdOffsetRange(3:5, -2)
OffsetArrays.IdOffsetRange(values=1:3, indices=-1:1)

julia> r isa AbstractUnitRange
true

julia> first(r)
1

julia> firstindex(r)
-1

See https://juliaarrays.github.io/OffsetArrays.jl/stable/internals/#The-axes-of-OffsetArrays and the "fundamental axiom of generalized indexing."

Easy fix: add a Base.require_one_based_indexing(r).

@mkitti
Copy link
Contributor Author

mkitti commented Aug 19, 2021

What does s[OffsetArray(1:3, -1:1)] do?

The first question should be, "what did it do?". Here's the behavior on Julia 1.6.0:

julia> VERSION
v"1.6.0"

julia> using OffsetArrays

julia> s = "Tim Holy"
"Tim Holy"

julia> r = OffsetArrays.IdOffsetRange(3:5, -2)
OffsetArrays.IdOffsetRange(values=1:3, indices=-1:1)

julia> s[r]
"Tim"

julia> s[OffsetArray(1:3, -1:1)]
"Tim"

julia> @which s[r]
getindex(s::AbstractString, v::AbstractVector{var"#s79"} where var"#s79"<:Integer) in Base at strings/basic.jl:192

julia> Base.getindex(s::String, r::AbstractUnitRange{<:Integer}) = s[Int(first(r)):Int(last(r))]

julia> s[r]
"Tim"

julia> @which s[r]
getindex(s::String, r::AbstractUnitRange{var"#s1"} where var"#s1"<:Integer) in Main at REPL[11]:1

@timholy
Copy link
Member

timholy commented Aug 19, 2021

Interesting! A bit unexpected, but I guess there is an argument for strings that simply says they don't pay attention to the indexes of the indexes, but only to the values. 👍

@mkitti
Copy link
Contributor Author

mkitti commented Aug 20, 2021

@timholy you may also be interested in #39241 (comment) (merged),
#41805, #41809, and #41810 (approved by @vtjnash, pending merge)

@vtjnash
Copy link
Member

vtjnash commented Aug 20, 2021

Strings also don't pay attention to the values either, just the end points. They don't really act like arrays when indexed.

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Aug 25, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants