search

JuliaCollections / DataStructures.jl

Julia implementation of Data structures
https://juliacollections.github.io/DataStructures.jl/latest/
MIT License
692 stars 246 forks source link

Separate Error struct for heaps Bound Error #598

Open karan0299 opened 4 years ago

karan0299 commented 4 years ago

Currently for bounds error in heaps , array bounds error is used as 

julia> x = MutableBinaryMinHeap{Int}()
MutableBinaryHeap()
julia> push!(x,10)
1
julia> delete!(x,2)
ERROR: BoundsError: attempt to access 1-element Array{Int32,1} at index [2]
Stacktrace:
 [1] getindex at ./array.jl:731 [inlined]
 [2] delete!(::MutableBinaryHeap{Int32,DataStructures.LessThan}, ::Int32) at /home/kps/DataStructures.jl/src/heaps/mutable_binary_heap.jl:271
 [3] top-level scope at none:0

Instead BoundsError for heaps to be displayed as

ERROR: HeapBoundsError: attempt to access 1-element MutableBinaryMinheap{Int32} at index [2]
eulerkochy commented 4 years ago

I will pass on what I feel about this. On one hand, heaps here are built over arrays, so the Stacktrace report and BoundsError looks just fine. On the other hand, you are proposing a layer of indirection to BoundsError, specifically for the case of heaps. If that's the case, it would make sense to propose the same for every data-structure in cases where it attempts to access non-existent elements. Now that's something, I haven't seen throughout this codebase. It's something to be pondered upon !

karan0299 commented 4 years ago

Yeah, I agree with you on that point that it to propose the same for every data-structure. I think it is better if we provide abstraction to user using particular datastructure.