Improve bounds checks in heap operations #954
Conversation
|
looks good from a quick glance, though i would like to look again more carefully. |
| rev = Base.ReverseOrdering(ord) | ||
|
|
||
| buffer = heapify(arr[1:n], rev) | ||
| buffer = heapify!(arr[1:n], rev) |
There was a problem hiding this comment.
This is safe since that slice is a copy. Good catch
|
|
||
| for i = n + 1 : length(arr) | ||
| @inbounds xi = arr[i] | ||
| @inbounds for i = n + 1 : length(arr) |
There was a problem hiding this comment.
Isn't this wrong? Since we do not know how arr is going to be indexed?
Or do we know?
If so it was wrong before, but it is still wrong now.
If we want this i think we need a Base.require_one_based_indexing(arr) ?
There was a problem hiding this comment.
Thanks for reviewing this! I was sprinkling Base.require_one_based_indexing around, but then I realized that any array with non-standard indexing would not be compatible with:
# Binary heap indexing
heapleft(i::Integer) = 2i
heapright(i::Integer) = 2i + 1
heapparent(i::Integer) = div(i, 2)So I wonder if the entire arrays-as-heaps.jl system should be documented as only compatible with one-based indexing and all these checks dropped. What do you think?
There was a problem hiding this comment.
I would keep the checks, and optionally document it.
the checks are more important than docs.
|
Great, can you increment this to v0.19.2 so I can tag a release after merge then this should be good to go in |
|
Done! Thank you! |
|
|
||
| # Binary min-heap percolate down. | ||
| function percolate_down!(xs::AbstractArray, i::Integer, x=xs[i], o::Ordering=Forward, len::Integer=length(xs)) | ||
| Base.@propagate_inbounds function percolate_down!(xs::AbstractArray, i::Integer, x, o::Ordering=Forward, len::Integer=length(xs)) |
There was a problem hiding this comment.
This is a breaking change? It seems you made the third argument mandatory.
There was a problem hiding this comment.
Ah, thanks for letting me know and apologies for the mistake. I think this could be fixed by adding a default ordering argument to the method below, i.e.
- Base.@propagate_inbounds percolate_down!(xs::AbstractArray, i::Integer, o::Ordering, len::Integer=length(xs)) = percolate_down!(xs, i, xs[i], o, len)
+ Base.@propagate_inbounds percolate_down!(xs::AbstractArray, i::Integer, o::Ordering=Forward, len::Integer=length(xs)) = percolate_down!(xs, i, xs[i], o, len)Do you agree, @oxinabox?
There was a problem hiding this comment.
I think the PR above is in good shape to be merged and fix this issue. Sorry everyone, I didn't realize the method was de facto public, so I wasn't thinking about its impact on compatibility.
3 participants
No description provided.