-
Couldn't load subscription status.
- Fork 261
Improve MutableLinkedList for 1.0 #873
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor feedback.
I have given you merge rights so merge this when you are happy and mark this file done in the issue
| function Base.delete!(l::MutableLinkedList, idx) | ||
| Expr(:meta, :noinline) | ||
| Base.depwarn("`delete!(l::MutableLinkedList,idx::Int)` is deprecated, use `deleteat!(l,idx)` instead.", :delete!) | ||
| deleteat!(l,idx) | ||
| end | ||
|
|
||
| function Base.delete!(l::MutableLinkedList, r::UnitRange) | ||
| Expr(:meta, :noinline) | ||
| Base.depwarn("`delete!(l::MutableLinkedList,r::UnitRange)` is deprecated, use `deleteat!(l,idx)` instead.", :delete!) | ||
| deleteat!(l,r) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this be done with the @deprecate macro?
| Base.length(l::MutableLinkedList) = l.len | ||
| Base.collect(l::MutableLinkedList{T}) where T = T[x for x in l] | ||
| Base.eltype(::Type{<:MutableLinkedList{T}}) where T = T | ||
| Base.collect(l::MutableLinkedList{T}) where {T} = T[x for x in l] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
I would have thought it would fall back to this if we didn't define it.
| end | ||
|
|
||
| function Base.map(f::Base.Callable, l::MutableLinkedList{T}) where T | ||
| function Base.map(f::Base.Callable, l::MutableLinkedList{T}) where {T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to strip out a lot of these constraints that do not follow modern practices.
| function Base.map(f::Base.Callable, l::MutableLinkedList{T}) where {T} | |
| function Base.map(f, l::MutableLinkedList{T}) where {T} |
| end | ||
|
|
||
| function Base.filter(f::Function, l::MutableLinkedList{T}) where T | ||
| function Base.filter(f::Function, l::MutableLinkedList{T}) where {T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function Base.filter(f::Function, l::MutableLinkedList{T}) where {T} | |
| function Base.filter(f, l::MutableLinkedList{T}) where {T} |
Thanks for the review. As soon as I've time I'll look into your recommendations. |
|
Hi! Is this PR alive? I am running into issues, where Thank you! |
It started as a simple inspection into MutableLinkedList and ended into a thorough refactoring of the code. As a consequence bugs have been fixed, and the API is now in alignment with Base.
_insert,_remove!,_traverse)@boundscheck: traversal (=indexing) is O(n), so the@boundscheckare not going to be a big deal.show()is now limited to the space given and has the same apperance asVectorVector{T}()append!(l,collections...)filter!(f,l)reverse!(l)empty!(l)splice!(l,idx)splice!(l,range)there is another PR for splice! but I found that it was flawedMutableLinkedList(iter)without a type parameter infers the eltype fromiterlastandfirstreturn a BoundsError if the list is empty instead of anArgumentErrorto mirrorVectorpopat!(l,idx)throws aBoundsErrorif the list is emtpy instead of anArgumentErrorto mirrorVectordelete!has been deprecated todeleteat!deleteappend!()will not mutate the last elementFixes
MutableLinkedList's #794Supersedes:
In case this gets merge it needs to be squashed, the single commits aren't useful on their own.