Add Heap.removeAll(where:) #454
Conversation
lorentey
left a comment
lorentey
left a comment
There was a problem hiding this comment.
Thanks; adding this is a good idea!
The implementation needs a bit of improvement.
Note that this project uses Swift stdlib indentation rules: our basic unit of indentation is two spaces (not four, not three). The unit of indentation must be kept consistent in the entire project.
Sources/HeapModule/Heap.swift
Outdated
|
|
||
| /// Removes all the elements that satisfy the given predicate. | ||
| /// | ||
| /// The heap *must not* be empty. |
There was a problem hiding this comment.
I believe I made this decision based off of the methods like replaceMax, however, now looking at other removeAll methods this is never a precondition so I believe the right decision is to not have this precondition.
Sources/HeapModule/Heap.swift
Outdated
| ) rethrows { | ||
| // Precondition: the heap must not be empty. | ||
| precondition(!isEmpty, "Heap must not be empty") | ||
| _storage = try _storage.filter { try !shouldBeRemoved($0) } |
There was a problem hiding this comment.
This method needs to forward to the standard MutableCollection/RangeReplaceableCollection method that implements the same.
| _storage = try _storage.filter { try !shouldBeRemoved($0) } | |
| _storage = try _storage.removeAll(where: shouldBeRemoved) |
Sources/HeapModule/Heap.swift
Outdated
| public mutating func removeAll( | ||
| where shouldBeRemoved: (Element) throws -> Bool | ||
| ) rethrows { | ||
| // Precondition: the heap must not be empty. |
There was a problem hiding this comment.
What would you say is the purpose of this comment?
There was a problem hiding this comment.
Seems quite redundant now that its pointed it out haha
Sources/HeapModule/Heap.swift
Outdated
| where shouldBeRemoved: (Element) throws -> Bool | ||
| ) rethrows { | ||
| // Precondition: the heap must not be empty. | ||
| precondition(!isEmpty, "Heap must not be empty") |
There was a problem hiding this comment.
What's the reason for adding this precondition?
Sources/HeapModule/Heap.swift
Outdated
| _update { handle in | ||
| handle.heapify() | ||
| } | ||
| } |
There was a problem hiding this comment.
What happened to the indentation here?
There was a problem hiding this comment.
Sorry about that, will go back and fix style.
| handle.heapify() | ||
| } | ||
| } | ||
| _checkInvariants() |
There was a problem hiding this comment.
If invariant checking is enabled, then it needs to be done on every exit path. The way to do that is by nesting this call in a defer block, and putting it on the top of the function, like the other operations do.
Tests/HeapTests/HeapTests.swift
Outdated
| } | ||
|
|
||
| func test_removeAll_noneRemoved() { | ||
| var heap = Heap<Int>((1...10).shuffled()) |
There was a problem hiding this comment.
Two issues:
- Our tests need to have repeatable results; truly random shuffling is not acceptable.
- (minor) While there is nothing wrong with only looking at a single count, it's almost trivial to try a range of counts, so let's just do that!
The other test cases show how to produce reproducibly random input for such purposes, and how to try many different counts.
Tests/HeapTests/HeapTests.swift
Outdated
|
|
||
| func test_removeAll_noneRemoved() { | ||
| var heap = Heap<Int>((1...10).shuffled()) | ||
| let originalSorted = heap.unordered.sorted() |
There was a problem hiding this comment.
It pays to be paranoid with tests -- the expected results must not be tainted by going through the same construct that the test is verifying. We know that the heap holds the elements 1 ... 10; we don't need to ask the heap to feed them back to us, and then sort the result ourselves.
let expected = 1 ... 10
heap.removeAll { _ in false }
expectEqualElements(heap.itemsInAscendingOrder(), expected)
|
@swift-ci test |
2 participants
This PR introduces a new method
Heap.removeAll(where:)that allows users to remove all elements satisfying a given predicate from the heap in O(n) time. This is done by filtering the buffer and re-heapifying.Closes #255
Checklist