std: Make std.PriorityQueue and std.PriorityDequeue unmanaged containers#25559
std: Make std.PriorityQueue and std.PriorityDequeue unmanaged containers#25559saurabh-mish wants to merge 22 commits intoziglang:masterfrom
std.PriorityQueue and std.PriorityDequeue unmanaged containers#25559Conversation
… and update tests
…ecked`, `peekMin`, `removeMin`, and `removeMax`
…d `removeMax` to `popMax`; update tests
|
Unrelated to these changes, but it seems theres'a typo in the doc comment: (The wording is a bit clunky imo, I'd personally just replace the last part with "otherwise", or drop it since it's logically implied - but up to you or maintainers to decide.) |
Summary of changesPriority Dequeue
Moved Priority Dequeue into Priority DequePriority Queue
TODO (this part will be edited based on progress, feedback, and observation)
|
… `.empty` value and use it in tests
|
Since there's already |
I think so too, pushing |
…st) and `capacity` method to `getCapacity`
lib/std/priority_dequeue.zig
Outdated
| /// Returns `true` if the queue is empty and `false if not. | ||
| pub fn isEmpty(self: Self) bool { | ||
| return if (self.len > 0) false else true; | ||
| } |
There was a problem hiding this comment.
IMHO we should not move simple checks like self.len > 0 to a function. It would have been needed if the empty state was complex or required some additional checks. Because it is just a length check, by adding this helper function we are actually making code slower, but without substantial readability improvements.
http://ithare.com/wp-content/uploads/part101_infographics_v08.png
There was a problem hiding this comment.
Small functions can (and are expected to) be trivially inlined by the compiler's optimization pass.
There was a problem hiding this comment.
Adding an isEmpty method was intentional because its commonly used in general purpose programming. After adding it, I thought of actually using it in existing methods.
There was a problem hiding this comment.
I don't think an isEmpty method is worth adding, they don't exist for other containers and len checks are simple.
There was a problem hiding this comment.
Also weird implementation that doesn't just return self.len == 0;
There was a problem hiding this comment.
Fixed isEmpty methods of both containers
There was a problem hiding this comment.
I don't think an
isEmptymethod is worth adding, they don't exist for other containers andlenchecks are simple.
I feel that methods which provide information about a field are easier to read than directly accessing it.
For example, a method count already exists in both these containers, which is easier to read than items.len.
Similarly, the newly added isEmpty method is easier to read than items.len == 0.
…`, add `.empty` value and use it in tests
…ethods and tests to deque, add doc comments for fields
|
This PR is ready for review (updated the summary of changes). |
#23431 had been dormant for a while.
This PR revives #21433 and resolves #21432.