DataStructures.jl icon indicating copy to clipboard operation
DataStructures.jl copied to clipboard

Improve MutableLinkedList for 1.0

Open laborg opened this issue 2 years ago • 2 comments
trafficstars

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.

  • refactored: reduce code duplication by introducin a couple of functions basic (_insert, _remove!, _traverse)
  • refactored: removed @boundscheck: traversal (=indexing) is O(n), so the @boundscheck are not going to be a big deal.
  • improvement: depending on the index it will either traverse the list from front or from the back
  • improvement: show() is now limited to the space given and has the same apperance as Vector
  • added: verbose, readable tests for functions and a comparison of all functions against Vector{T}()
  • added: append!(l,collections...)
  • added: filter!(f,l)
  • added: reverse!(l)
  • added: empty!(l)
  • added: splice!(l,idx)
  • added: splice!(l,range) there is another PR for splice! but I found that it was flawed
  • added: constructor MutableLinkedList(iter) without a type parameter infers the eltype from iter
  • changed: last and first return a BoundsError if the list is empty instead of an ArgumentError to mirror Vector
  • changed: popat!(l,idx) throws a BoundsError if the list is emtpy instead of an ArgumentError to mirror Vector
  • deprecated: delete! has been deprecated to deleteat!
  • fixed: allow i:j with j <= i for delete
  • fixed: append!() will not mutate the last element

Fixes

  • https://github.com/JuliaCollections/DataStructures.jl/issues/794
  • https://github.com/JuliaCollections/DataStructures.jl/issues/795
  • https://github.com/JuliaCollections/DataStructures.jl/issues/739
  • https://github.com/JuliaCollections/DataStructures.jl/issues/704

Supersedes:

  • https://github.com/JuliaCollections/DataStructures.jl/pull/871
  • https://github.com/JuliaCollections/DataStructures.jl/pull/818

In case this gets merge it needs to be squashed, the single commits aren't useful on their own.

laborg avatar Oct 13 '23 09:10 laborg

minor feedback. I have given you merge rights so merge this when you are happy and mark this file done in the issue

Thanks for the review. As soon as I've time I'll look into your recommendations. ~~In the meantime: I can't merge the PR via user interface. Shouldn't this work since you gave me merge rights?~~ Nevermind, the existing conflicts will probably prevent merging.

laborg avatar Oct 26 '23 04:10 laborg