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

Add is_full() to limited. #6

Merged
merged 2 commits into from
Jul 5, 2022
Merged

Conversation

Carter12s
Copy link
Contributor

is_full() is a useful function and an existing method of the underlying crossbeam queue type. I believe exposing it has limited / no downsides and slightly improves usability.

Additionally expanded some doc strings around len() vs capacity() where I had initial (minor) confusion.

@Carter12s
Copy link
Contributor Author

Note: I did look into adding is_full() to "Resizable", but implementation was non-obvious to me.

@bikeshedder
Copy link
Owner

For resizable you can just implement it like this:

pub fn is_full(&self) -> bool {
    self.len() >= self.capacity()
}

In theory it can give wrong values as it involves two atomic values which are read non-atomically, but it's as good as it can be implemented. It will only be wrong under rare circumstances: push/pop while a resize is happening

is_full() should never be used to decide whether to push or not. Any real world code should just call try_push instead.

Could you add this to the PR and while you're at it add a 0.2.3 - unreleased to the CHANGELOG documenting those changes?

@bikeshedder
Copy link
Owner

Also please add a test for it. This feature is trivial for the limited implementation so a test won't be necessary here. At least theresizable contains a small amount of logic and a test would be "nice to have". I'll also merge it without tests. It would just be the cherry on top. 😉

@Carter12s
Copy link
Contributor Author

Excellent suggestions. Pushed changes with updated changelog, resizable impl of is_full, and a (basic) test case for resizable.

@bikeshedder bikeshedder merged commit 73fb85e into bikeshedder:master Jul 5, 2022
@bikeshedder
Copy link
Owner

Thanks a lot! 👍

I just uploaded version 0.2.3 to crates.io: https://crates.io/crates/deadqueue/0.2.3

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