From 1c16afdfd1eeed12b78f610f8cbb4bfe0f2bdb8f Mon Sep 17 00:00:00 2001 From: Andy Ferris Date: Fri, 1 May 2020 05:55:41 +1000 Subject: [PATCH] Make view of range with range be a range (#26872) * Make view of range with range be a range Typical `AbstractRange` types are compact and immutable, and we can remove the overhead of `SubArray` in this case while the cost of `view` remains O(1). Here we specialize only on valid combinations of integer ranges, otherwise still relying on `SubArray`. * Cover all implementations of `getindex(::AbstractRange,...)` * Add NEWS Co-authored-by: Andy Ferris Co-authored-by: Matt Bauman --- NEWS.md | 2 ++ base/subarray.jl | 26 ++++++++++++++++++++++++++ test/ranges.jl | 16 ++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/NEWS.md b/NEWS.md index 5d65d9f5a3769..0052496a76f3f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -143,6 +143,8 @@ Standard library changes * A 1-d `Zip` iterator (where `Base.IteratorSize` is `Base.HasShape{1}()`) with defined length of `n` has now also size of `(n,)` (instead of throwing an error with truncated iterators) ([#29927]). * The `@timed` macro now returns a `NamedTuple` ([#34149]) * New `supertypes(T)` function returns a tuple of all supertypes of `T` ([#34419]). +* Views of builtin ranges are now recomputed ranges (like indexing returns) instead of + `SubArray`s ([#26872]). * Sorting-related functions such as `sort` that take the keyword arguments `lt`, `rev`, `order` and `by` now do not discard `order` if `by` or `lt` are passed. In the former case, the order from `order` is used to compare the values of `by(element)`. In the latter case, diff --git a/base/subarray.jl b/base/subarray.jl index 638cdcbe402e3..a540ef1811e36 100644 --- a/base/subarray.jl +++ b/base/subarray.jl @@ -164,6 +164,32 @@ function view(A::AbstractArray, I::Vararg{Any,N}) where {N} unsafe_view(_maybe_reshape_parent(A, index_ndims(J...)), J...) end +# Ranges implement getindex to return recomputed ranges; use that for views, too (when possible) +function view(r1::OneTo, r2::OneTo) + @_propagate_inbounds_meta + getindex(r1, r2) +end +function view(r1::AbstractUnitRange, r2::AbstractUnitRange{<:Integer}) + @_propagate_inbounds_meta + getindex(r1, r2) +end +function view(r1::AbstractUnitRange, r2::StepRange{<:Integer}) + @_propagate_inbounds_meta + getindex(r1, r2) +end +function view(r1::StepRange, r2::AbstractRange{<:Integer}) + @_propagate_inbounds_meta + getindex(r1, r2) +end +function view(r1::StepRangeLen, r2::OrdinalRange{<:Integer}) + @_propagate_inbounds_meta + getindex(r1, r2) +end +function view(r1::LinRange, r2::OrdinalRange{<:Integer}) + @_propagate_inbounds_meta + getindex(r1, r2) +end + function unsafe_view(A::AbstractArray, I::Vararg{ViewIndex,N}) where {N} @_inline_meta SubArray(A, I) diff --git a/test/ranges.jl b/test/ranges.jl index 33a9ad43424d6..91ad68db40c07 100644 --- a/test/ranges.jl +++ b/test/ranges.jl @@ -1401,6 +1401,22 @@ end @test x isa StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} end +@testset "Views of ranges" begin + @test view(Base.OneTo(10), Base.OneTo(5)) === Base.OneTo(5) + @test view(1:10, 1:5) === 1:5 + @test view(1:10, 1:2:5) === 1:2:5 + @test view(1:2:9, 1:5) === 1:2:9 + + # Ensure we don't hit a fallback `view` if there's a better `getindex` implementation + vmt = collect(methods(view, Tuple{AbstractRange, AbstractRange})) + for m in methods(getindex, Tuple{AbstractRange, AbstractRange}) + tt = Base.tuple_type_tail(m.sig) + tt == Tuple{AbstractArray,Vararg{Any,N}} where N && continue + vm = findfirst(sig->tt <: Base.tuple_type_tail(sig.sig), vmt) + @test vmt[vm].sig != Tuple{typeof(view),AbstractArray,Vararg{Any,N}} where N + end +end + @testset "Issue #26608" begin @test_throws BoundsError (Int8(-100):Int8(100))[400] @test_throws BoundsError (-100:100)[typemax(UInt)]