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.

Sign up for GitHub

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

make last work on reversible any collection #42991

Merged
merged 13 commits into from
Nov 10, 2021
Merged

make last work on reversible any collection #42991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8fa6fba
make `last` work on reversible any collection
MasonProtter Nov 7, 2021
33f5c1d
tweak docstring
MasonProtter Nov 7, 2021
8612a6d
Update NEWS.md
MasonProtter Nov 7, 2021
91cf714
Update iterators.jl
MasonProtter Nov 7, 2021
5a2dc61
delete whitespace
MasonProtter Nov 7, 2021
1953792
fix whitespace
MasonProtter Nov 7, 2021
2155299
Update abstractarray.jl
MasonProtter Nov 7, 2021
daa8b01
fix whitespace in compat entry
MasonProtter Nov 7, 2021
2a3df7c
try removing `first(x::Reverse)` method
MasonProtter Nov 7, 2021
be87a22
use `lastindex` for `last(::AbstractArray)`
MasonProtter Nov 7, 2021
dd55049
try with AbstractVector
MasonProtter Nov 7, 2021
bcdf540
add missing comma
MasonProtter Nov 8, 2021
492a9f4
Update base/abstractarray.jl
MasonProtter Nov 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Language changes
* `@time` and `@timev` now take an optional description to allow annotating the source of time reports.
i.e. `@time "Evaluating foo" foo()` ([#42431])
* New `@showtime` macro to show both the line being evaluated and the `@time` report ([#42431])
* `last(collection)` will now work on any collection that supports `Iterators.reverse` and `first`, rather than being
restricted to indexable collections.

Compiler/Runtime improvements
-----------------------------
Expand Down
11 changes: 8 additions & 3 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,11 +462,15 @@ end
last(coll)

Get the last element of an ordered collection, if it can be computed in O(1) time. This is
accomplished by calling [`lastindex`](@ref) to get the last index. Return the end
point of an [`AbstractRange`](@ref) even if it is empty.
accomplished by calling [`Iterators.reverse`](@ref) and then [`first`](@ref) on that reversed iterator.
Return the end point of an [`AbstractRange`](@ref) even if it is empty.

See also [`first`](@ref), [`endswith`](@ref).

!!! compat "Julia 1.8"
For versions of julia older than 1.8, `last(x)` will only work on collections that support indexing and
[`lastindex`](@ref).

# Examples
```jldoctest
julia> last(1:2:10)
Expand All @@ -476,7 +480,8 @@ julia> last([1; 2; 3; 4])
4
```
"""
last(a) = a[end]
last(a) = first(Iterators.reverse(a))
last(a::AbstractVector) = a[end]
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be for AbstractArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that above, but @simeonschaub mentioned that since there are abstract arrays that don't support linear indexing, this might be the better default. Can give AbstractArray a try though and see how it goes

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, all arrays support linear indexing, but it just isn't required to be the most efficient index

Copy link
Member

Choose a reason for hiding this comment

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

An array can also have a lastindex that isn't necessarily an integer.

Copy link
Member

Choose a reason for hiding this comment

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

I thought so too, but lastindex is actually defined to always return an integer.


"""
last(itr, n::Integer)
Expand Down
1 change: 0 additions & 1 deletion base/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ size(r::Reverse) = size(r.itr)
IteratorSize(::Type{Reverse{T}}) where {T} = IteratorSize(T)
IteratorEltype(::Type{Reverse{T}}) where {T} = IteratorEltype(T)
last(r::Reverse) = first(r.itr) # the first shall be last
first(r::Reverse) = last(r.itr) # and the last shall be first
Copy link
Member

Choose a reason for hiding this comment

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

We might need to put this back, with a special attempt not to form an infinite recursion, but let's wait for PkgEval to determine that.


# reverse-order array iterators: assumes more-specialized Reverse for eachindex
@propagate_inbounds function iterate(A::Reverse{<:AbstractArray}, state=(reverse(eachindex(A.itr)),))
Expand Down
5 changes: 5 additions & 0 deletions test/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,3 +883,8 @@ end
@test Iterators.peel(x^2 for x in 2:4)[1] == 4
@test Iterators.peel(x^2 for x in 2:4)[2] |> collect == [9, 16]
end

@testset "last for iterators" begin
@test last(Iterators.map(identity, 1:3)) == 3
@test last(Iterators.filter(iseven, (Iterators.map(identity, 1:3)))) == 2
end