Skip to content

Conversation

@dragazo
Copy link

@dragazo dragazo commented Aug 25, 2023

Closes #47.

As far as I can tell, there was no real need to use MaybeUninit<InlineString> instead of just InlineString directly, due to it being a POD type. All that was required was changing the types of pointer casts we do to directly use &self.data as *const _ rather than self.data.as_ptr() in various places.

Due to no longer using the MaybeUninit union type, we are able to get Option size optimization out of the InlineString. To do this, I changed Marker(u8) to Marker(NonZeroU8) and changed it to use the low 2 bits for storing both the discriminant (as it already did), and also a non-zero bit to guarantee the requirement of NonZeroU8. I've also changed the allocator alignment requirement to 4 instead of 2 to guarantee these bits are free to use; this should not cause issues on 32 or 64 bit systems, which are our targets.

To ensure that a BoxedString write over the memory of the InlineString does not violate the requirements of NonZeroU8 (and/or produce a None value), the ptr field in BoxedString was changed to a new TaggedPtr type that automatically handles setting the low 2 bits appropriately (and removing them when queried).

I feel that additional testing should be done, but it currently passes all existing test cases and a few that I added specifically about size requirements and Some vs None checks for small and large strings.

@dragazo
Copy link
Author

dragazo commented Aug 26, 2023

I also just took the liberty of solving #33 by using alloc::borrow::Cow, which is available in no_std mode.

@dragazo
Copy link
Author

dragazo commented Aug 26, 2023

I'll also note that this PR now uses 2 bits for the expanded discriminant, but only 3 states (None, InlineString, and BoxedString). The fourth pattern could be used to implement a feature like storing a &'static str like some other sso libs can do. That's outside the scope of this PR, though.

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.

Size optimization for Option

1 participant