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

ensure len and capacity methods reflect their intentions #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

neevek
Copy link

@neevek neevek commented Apr 4, 2023

No description provided.

@link2xt
Copy link
Collaborator

link2xt commented Apr 5, 2023

The implementation of Realloc trait for Vec should also be replaced to operate on the vector capacity rather than filling the vector with Default values. Also proper documentation comments are needed to describe what these methods do. Then this can be released as a major release.

I opened #5 suitable for a minor bugfix release, but I agree that .capacity() implementation in a Vec trait should not call .len().

@link2xt
Copy link
Collaborator

link2xt commented Apr 6, 2023

@neevek See discussion at async-email/async-imap#77 (comment), maybe it is not needed to have additional method because poolable vector is supposed to have exactly the same length and capacity all the time. I don't know what is the idea about HashMap though, we definitely cannot pre-fill it with default values as we cannot generate "default keys".

@flub Maybe we drop HashMap support completely, is anyone using it? It is inconsistent with what Vec does, because it reallocates capacity while Poolable::capacity() looks at the length. Maybe we better focus this crate at allocating Vec only or even Vec only.

src/poolable.rs Outdated
fn alloc(size: usize) -> Self {
vec![T::default(); size]
Vec::<T>::with_capacity(size)
Copy link
Collaborator

@link2xt link2xt Apr 6, 2023

Choose a reason for hiding this comment

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

This changes semantics quite a bit and breaks len == capacity invariant mentioned in async-email/async-imap#77 (comment)
I think this line should be reverted.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that this completely changes the original semantics, and that indeed will break code that uses the crate.
How about adding another alloc function that only reserves the requested size of memory, and doesn't fill the buffer with default values, which makes len work the way I intended, for example adding a reserve associated function, code that relies on alloc are not affected, and new code can choose to use alloc or reserve.

Copy link
Author

Choose a reason for hiding this comment

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

If that is okay, we can keep the implementation of the Realloc trait unchanged, and add a Reserve trait that provides a reserve function for poolable objects, or maybe we don't even need to do that since only Vec is used as poolable object.

@link2xt
Copy link
Collaborator

link2xt commented Apr 6, 2023

This was explicitly changed in f4c91d7 with a comment, so I am not sure we should just revert it. There is a PR #1 with more reasoning.

@neevek
Copy link
Author

neevek commented Apr 6, 2023

@link2xt I use byte-pool as an alternative to Vec<u8> just for having its poolable feature, so I can simply push data to the buffer and it grows its capacity as needed. Since the buffer object Block is Derefed to a Vec, I think Vec's features should be retained. In my use case, I need to check how much data have been pushed into the buffer, and I also need to slice the buffer, so len and capacity mean different things.

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