Conversation
| match capacity.checked_mul(2) { | ||
| Some(cap) => cap, | ||
| None => capacity, | ||
| } |
| } | ||
|
|
||
| pub fn make_layout<T>(capacity: usize, alignment: usize) -> alloc::alloc::Layout { | ||
| pub fn make_layout(capacity: usize, alignment: usize, type_size: usize) -> alloc::alloc::Layout { |
There was a problem hiding this comment.
It's not clear to me what we gain by removing the generic parameter here.
There was a problem hiding this comment.
It removes number of instantiations for function, reducing compile times.
If you're curious std library does the same https://github.com/rust-lang/rust/blob/207c80f105282245d93024c95ac408c622f70114/library/alloc/src/raw_vec.rs#L442-L445
By extracting as much code out of generics, you improve compile times automatically equivalent to number of possible T in your code
There was a problem hiding this comment.
Hmm.
Alright, we can explore this later.
This PR should focus solely on adding the two new associated functions and the tests.
If we want to work on reducing code bloat, we should first get some metrics set up and then we'll know if our refactors are actually doing anything or not.
| #[inline] | ||
| #[allow(clippy::cast_ptr_alignment)] | ||
| fn is_default(buf: core::ptr::NonNull<u8>) -> bool { | ||
| core::ptr::eq(buf.as_ptr(), &DEFAULT_U8) | ||
| } |
There was a problem hiding this comment.
I don't understand the purpose of this refactor either.
There was a problem hiding this comment.
To use outside of generic context
There was a problem hiding this comment.
To try and reduce code bloat again?
| struct Grow { | ||
| buf: core::ptr::NonNull<u8>, | ||
| old_capacity: usize, | ||
| new_capacity: usize, | ||
| alignment: usize, | ||
| len: usize, | ||
| type_size: usize, | ||
| } |
There was a problem hiding this comment.
I don't get why we have a Grow struct that also contains a type_size.
I'm not sure I understand this refactoring in general.
We should basically just update fn grow() to return a Option<()> instead of what we're doing now which is invoking handle_alloc_error.
To this end, the current usages just need to match on grow()'s new return type and invoke handle_alloc_error there and then the try_reserve_* associated functions can match on the None and return the Result type.
This should be largely good enough because any other allocation failures (for example, our layout is invalid) would be caused by bugs in the library.
There was a problem hiding this comment.
This Grow struct here is to just to make sure not to make mistake when passing arguments to grow function as I moved it outside of Vec as part of refactoring.
This is ofc your choice if you do not need refactoring to remove unneeded generic code.
As you said it would be simply done by refactoring original grow function
There was a problem hiding this comment.
To me it seems silly to create a struct just to give it an impl with a single associated function.
This pattern, imo, fits better for DropGuard-like types.
| #[test] | ||
| fn minivec_should_fail_try_reserve_impossible() { | ||
| let mut buf = minivec::mini_vec![0; 512]; | ||
| buf.try_reserve(usize::max_value()).expect_err("FAIL"); | ||
| } |
There was a problem hiding this comment.
This test is incomplete.
We've tested failure which is good but we need to test the success arm as well.
I also don't see try_reserve_exact() here either.
There was a problem hiding this comment.
There was a problem hiding this comment.
This is good but we also need to comment back in the tests in vec.rs as well. Seems like there's a good bit of tests the stdlib has there.
2 participants
This PR makes following changes:
Fixes #32