Skip to content

Commit

Permalink
Make view of a string return a substring (#35879)
Browse files Browse the repository at this point in the history
* make taking views of string indexing give substrings

* add news

* Remove excess whitespace

* handle single character string views correctly

* test values of views

* make substring etc work on AbstractUnitRanges

* Fix missing space
  • Loading branch information
oxinabox authored May 20, 2020
1 parent 9d70d45 commit 9655c21
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 1 deletion.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ Standard library changes
------------------------
* The `nextprod` function now accepts tuples and other array types for its first argument ([#35791]).
* The function `isapprox(x,y)` now accepts the `norm` keyword argument also for numeric (i.e., non-array) arguments `x` and `y` ([#35883]).
* `view`, `@view`, and `@views` now work on `AbstractString`s, returning a `SubString` when appropriate ([#35879]).
* All `AbstractUnitRange{<:Integer}`s now work with `SubString`, `view`, `@view` and `@views` on strings ([#35879]).

#### LinearAlgebra

Expand Down
6 changes: 5 additions & 1 deletion base/strings/substring.jl
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ end

@propagate_inbounds SubString(s::T, i::Int, j::Int) where {T<:AbstractString} = SubString{T}(s, i, j)
@propagate_inbounds SubString(s::AbstractString, i::Integer, j::Integer=lastindex(s)) = SubString(s, Int(i), Int(j))
@propagate_inbounds SubString(s::AbstractString, r::UnitRange{<:Integer}) = SubString(s, first(r), last(r))
@propagate_inbounds SubString(s::AbstractString, r::AbstractUnitRange{<:Integer}) = SubString(s, first(r), last(r))

@propagate_inbounds function SubString(s::SubString, i::Int, j::Int)
@boundscheck i j && checkbounds(s, i:j)
Expand All @@ -47,6 +47,10 @@ end
SubString(s::AbstractString) = SubString(s, 1, lastindex(s))
SubString{T}(s::T) where {T<:AbstractString} = SubString{T}(s, 1, lastindex(s))

@propagate_inbounds view(s::AbstractString, r::AbstractUnitRange{<:Integer}) = SubString(s, r)
@propagate_inbounds maybeview(s::AbstractString, r::AbstractUnitRange{<:Integer}) = view(s, r)
@propagate_inbounds maybeview(s::AbstractString, args...) = getindex(s, args...)

convert(::Type{SubString{S}}, s::AbstractString) where {S<:AbstractString} =
SubString(convert(S, s))
convert(::Type{T}, s::T) where {T<:SubString} = s
Expand Down
31 changes: 31 additions & 0 deletions test/strings/basic.jl
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,37 @@ end
@test endswith(z)(z)
end

@testset "SubStrings and Views" begin
x = "abcdefg"
@testset "basic unit range" begin
@test SubString(x, 2:4) == "bcd"
@test view(x, 2:4) == "bcd"
@test view(x, 2:4) isa SubString
@test (@view x[4:end]) == "defg"
@test (@view x[4:end]) isa SubString
end

@testset "other AbstractUnitRanges" begin
@test SubString(x, Base.OneTo(3)) == "abc"
@test view(x, Base.OneTo(4)) == "abcd"
@test view(x, Base.OneTo(4)) isa SubString
end

@testset "views but not view" begin
# We don't (at present) make non-contiguous SubStrings with views
@test_throws MethodError (@view x[[1,3,5]])
@test (@views (x[[1,3,5]])) isa String

# We don't (at present) make single character SubStrings with views
@test_throws MethodError (@view x[3])
@test (@views (x[3])) isa Char

@test (@views (x[3], x[1:2], x[[1,4]])) isa Tuple{Char, SubString, String}
@test (@views (x[3], x[1:2], x[[1,4]])) == ('c', "ab", "ad")
end
end


@testset "filter specialization on String issue #32460" begin
@test filter(x -> x ['', 'Ï', 'z', 'ξ'],
GenericString("J'étais n작작é pour plaiÏre à toute âξme un peu fière")) ==
Expand Down

4 comments on commit 9655c21

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - possible issues were detected. A full report can be found here. cc @maleadt

Please sign in to comment.