Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

collection priority queue: head should be higher priority than tail #80

Merged

Conversation

vagabondkjo
Copy link
Contributor

This PR makes some changes as below

1. collection priority queue

  • head should be higher priority than tail

2. add sint64 headIndex(const id& pov, sint64 maxPriority)

  • return elementIndex of first element with priority <= maxPriority in priority queue of pov (or NULL_INDEX if pov is unknown).

3. add sint64 tailIndex(const id& pov, sint64 minPriority)

  • return elementIndex of last element with priority >= minPriority in priority queue of pov (or NULL_INDEX if pov is unknown).

- the head should be higher priority than the tail
@philippwerner
Copy link
Contributor

Thanks! Looks good at the first glimpse, but please add some more tests to improve the test coverage and make sure that the code works correctly on more cases. These breakpoints are never reached in the tests:
grafik
grafik

@vagabondkjo vagabondkjo force-pushed the kjo/collection_priority_order branch from b401006 to 2e8f4d0 Compare March 26, 2024 05:24
@vagabondkjo
Copy link
Contributor Author

Thanks! Looks good at the first glimpse, but please add some more tests to improve the test coverage and make sure that the code works correctly on more cases. These breakpoints are never reached in the tests:

@philippwerner thanks for your comment.

I make some changes for _headIndex/_tailIndex, I think it's better than previous implementation. And, I also add more unittests.
Please help to review the PR again. Thanks!

@philippwerner philippwerner merged commit 2e64e7a into qubic:develop Mar 26, 2024
1 check passed
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.

2 participants