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 try_push_uninit and test #278

Closed
wants to merge 2 commits into from
Closed

Add try_push_uninit and test #278

wants to merge 2 commits into from

Conversation

phughk
Copy link

@phughk phughk commented Oct 15, 2024

Add try_push_uninit , which tries to add an additional element and provides a raw pointer to the already allocated memory.

This is useful if the backing structure is larger than the stack size.

Included is a test that demonstrates the need for this functionality.

///
/// This is useful if the backed value is large and won't fit on stack.
///
/// Capacity error carries the data - since we don't have data then we need a different error type.
Copy link
Owner

Choose a reason for hiding this comment

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

please use CapacityError<()>

@bluss
Copy link
Owner

bluss commented Oct 16, 2024

Something like this is needed, but unsure about the right API

/// This is useful if the backed value is large and won't fit on stack.
///
/// Capacity error carries the data - since we don't have data then we need a different error type.
pub unsafe fn try_push_uninit(&mut self) -> Result<*mut T, ()> {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use a NonNull<T> for the pointer. While we think of the best shape of API for this.

let field_two = unsafe {&mut (*ptr).field_two};
*field_two = [2; 1_000_000];
let field_three = unsafe {&mut (*ptr).field_three};
*field_three = [3; 1_000_000];
Copy link
Owner

@bluss bluss Oct 16, 2024

Choose a reason for hiding this comment

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

Raw pointer is not a good interface because the user needs to remember to use field_three.write(...). Assigning with *field_three = ... only works for Copy types. The point is that the assignment first drops the existing value before copying (nominally) the new value into its place. MaybeUninit is a good place to look.

@tbu-
Copy link
Collaborator

tbu- commented Oct 16, 2024

Perhaps this could take the same shape as Vec::spare_capacity_mut?

@bluss
Copy link
Owner

bluss commented Oct 16, 2024

@tbu- that sounds good!

tbu- added a commit to tbu-/arrayvec that referenced this pull request Oct 16, 2024
This mirrors `Vec::spare_capacity_mut`. Description and example are
taken from this function, too.

CC bluss#278
tbu- added a commit to tbu-/arrayvec that referenced this pull request Oct 16, 2024
This mirrors `Vec::spare_capacity_mut`. Description and example are
taken from this function, too.

CC bluss#278
tbu- added a commit to tbu-/arrayvec that referenced this pull request Oct 16, 2024
This mirrors `Vec::spare_capacity_mut`. Description and example are
taken from this function, too.

CC bluss#278
bluss pushed a commit that referenced this pull request Oct 16, 2024
This mirrors `Vec::spare_capacity_mut`. Description and example are
taken from this function, too.

CC #278
@bluss
Copy link
Owner

bluss commented Oct 16, 2024

@tbu- was quick to add a suggestion in #279 which is a pretty nice interface - and more versatile in some ways - I hope that addresses the need here, what do you think?

@phughk
Copy link
Author

phughk commented Oct 16, 2024

@tbu- @bluss thank you! Yeah I'm not obsessed about the PR, I added this to a fork and used in my own project and wanted to contribute back. If you have a better API then very happy with the turnaround. Thank you as well for the diligent review; I learnt from it 😊

@phughk phughk closed this Oct 16, 2024
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.

3 participants