Skip to content

Conversation

saurabh-mish
Copy link
Contributor

@saurabh-mish saurabh-mish commented Oct 13, 2025

#23431 had been dormant for a while.

This PR revives #21433 and resolves #21432.

@rohlem
Copy link
Contributor

rohlem commented Oct 13, 2025

Unrelated to these changes, but it seems theres'a typo in the doc comment:
PriorityDeque says Order.gt if the third argument should be min-popped second,
while Queue says Order.gt if the third argument should be min-popped first.
From my understanding PriorityQueue is consistent here, the error for Deque was made in 22e1d92, but not in corresponding 82e8930.

(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.)

@saurabh-mish
Copy link
Contributor Author

saurabh-mish commented Oct 13, 2025

Summary of changes

Priority Dequeue

std.PriorityDequeue has method signatures similar to std.Deque

  • Moved methods removeMinOrNull into removeMin and removeMaxOrNull into removeMax; updated tests.

  • Renamed methods add, removeMin and removeMax to push, popMin and popMax repectively.

  • Added new method isEmpty which returns true if the priority dequeue is empty and false if not. It is being used in addUnchecked, peekMin, removeMin, and removeMax.

Priority Queue

std.PriorityQueue has method signatures similar to std.PriorityDequeue

  • Moved method removeOrNull into remove and updated corresponding tests.

  • Renamed methods add to push and remove to pop.

  • Added new method isEmpty method which returns true if the priority queue is empty and false if not. It is being used in peek and pop methods.

Moved Priority Dequeue into Priority Deque

TODO (this part will be edited based on progress, feedback, and observation)

  • Add / improve doc comments
    • PriorityQueue
    • PriorityDeque
  • Consistent fields: PriorityQueue has items, cap, and context, whereas PriorityDeque has items, len, context
    • Was struggling with this to be honest. If this is indeed an issue, I think it is beyond the scope of this PR.
  • .empty value
    • PriorityQueue
    • PriorityDeque

@linux-user36
Copy link

Since there's already deque.zig, should this maybe be renamed to PriorityDeque for consistency?

@saurabh-mish
Copy link
Contributor Author

Since there's already deque.zig, should this maybe be renamed to PriorityDeque for consistency?

I think so too, pushing PriorityDeque related changes shortly

…st) and `capacity` method to `getCapacity`
Comment on lines 54 to 57
/// 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;
}
Copy link
Contributor

@imomaliev imomaliev Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rohlem rohlem Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small functions can (and are expected to) be trivially inlined by the compiler's optimization pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@saurabh-mish
Copy link
Contributor Author

This PR is ready for review (updated the summary of changes).

@saurabh-mish saurabh-mish marked this pull request as ready for review October 13, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a PriorityQueueUnmanaged type to the standard library

4 participants