more efficient array indexing with AbstractUnitRange

[sanjibansg]

Fixes #39858
Relaxed method signature for getindex(A::Array, I:: UnitRange{Int}) which now has AbstractUnitRange{<:Integer} as the second argument.

[sostock]

Some tests should be added to check that indexing still works correctly for newly-allowed types of ranges, for example Base.OneTo, UnitRange{UInt} and UnitRange{BigInt}.

[jishnub]

Would be good to have this, @Kahanikaar could you add the tests that have been suggested? Not too sure where the tests should go, perhaps to test/abstractarray.jl? You may create a new testset block stating the issue number where you can add the tests. Have a look at the test files to get an idea.

[sanjibansg]

Would be good to have this, @Kahanikaar could you add the tests that have been suggested? Not too sure where the tests should go, perhaps to test/abstractarray.jl? You may create a new testset block stating the issue number where you can add the tests. Have a look at the test files to get an idea.

Sure, will look after this, will add the tests to test/abstractarray.jl

[sanjibansg]

Tests added as asked. Do review @sostock @jishnub

[sostock]

If the test arrays only contain ones, we don’t know that it actually copies the correct numbers. Better use rand or something like collect(1:10).

[sanjibansg]

If the test arrays only contain ones, we don’t know that it actually copies the correct numbers. Better use rand or something like collect(1:10).

Tests now use collect in place of ones.

[simeonschaub]

Thanks for the PR and welcome! Hope this hasn't been too unpleasant an experience, but looks good to me now.

[sanjibansg]

Thanks for the PR and welcome! Hope this hasn't been too unpleasant an experience, but looks good to me now.

Not at all. It was really great contributing to Julia. Thanks for all the help! @jishnub @sostock @simeonschaub

[sostock]

It might also be good to test that indexing preserves the type, i.e., typeof(a[x]) === typeof(a).

[jishnub]

This is not true in general though, although it probably holds for the range types in Base that use 1-based indexing. If this test is added, it's better to leave a comment specifying this. Types are perhaps an internal detail, and checking for types might be too strict a test. Checking for the values and the axes should suffice in most cases.

Eg cases where this doesn't hold (using OffsetArrays and StaticArrays):

julia> ones(10)[SOneTo(2)]
2-element SizedArray{Tuple{2},Float64,1,1,Array{Float64,1}} with indices SOneTo(2):
 1.0
 1.0

julia> ones(10)[Base.IdentityUnitRange(3:5)]
3-element OffsetArray(::Array{Float64,1}, 3:5) with eltype Float64 with indices 3:5:
 1.0
 1.0
 1.0

[jishnub]

@Kahanikaar is this ready? Waiting for this before merging JuliaArrays/OffsetArrays.jl#211

[sanjibansg]

@Kahanikaar is this ready? Waiting for this before merging JuliaArrays/OffsetArrays.jl#211

Commits are ready and approved, should I squash them now? @jishnub

[sostock]

Squashing isn’t necessary, the PR will be squashed when merging it. But it would be nice to resolve the merge conflict.

[sostock]

The conflict wasn’t resolved correctly, there is an end missing.

[jishnub]

Gentle bump, since this is ready perhaps this may be reviewed? Would be nice to have this merged as there is a demonstrable performance improvement here.