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! |
2 participants
No description provided.