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

Implement beef::lean::Cow on other pointer widths #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Waelwindows
Copy link

This should remove the restriction on beef::lean::Cow being only available on targets with a pointer width of 64

Waelwindows added 2 commits June 9, 2021 02:40
length/capacity will be evenly distributed irrespective of target pointer width
Update the documentation to reflect new behaviour on non-x64 platforms
@CAD97
Copy link

CAD97 commented Jun 9, 2021

This is, strictly speaking, a breaking change, since on 32 bit targets lean::Cow switches from supporting u32::MAX elements to u16::MAX elements. (On 16 bit pointer targets, it goes from u16::MAX to u8::MAX elements. Ignoring 16 bit pointers from hereon.)

There's advantages to both ways. With usize lean::Cow, you always fit the cow into 2×usize. With u64 lean::Cow, the cow always supports u32::MAX elements, which is enough for (nearly) anyone, since you'll (typically) run out of RAM first.

lean::Cow is more necessary on 64 bit architectures, where pointers are thicc quite large comparatively. It's reasonable to use lean::Cow so save the space on 64 bit targets by restricting the capacity as if it were a 32 bit target, but still wanting to keep the extra space on 32bit (which is still less than 64bit lean) to keep the same max capacity.

I think there is value in exposing the 2×usize version to 32 bit architectures, but it should be under a new reëxport (beef::leaner::Cow? skim::Cow?), and documentation would need to be made extra clear so that it's clear from both 64bit and 32bit docs what the behavior is on both targets.

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