Skip to content

Use 16-bit lengths on 16-bit targets#234

Merged
tbu- merged 1 commit into
bluss:masterfrom
kornelski:u16
Oct 17, 2024
Merged

Use 16-bit lengths on 16-bit targets#234
tbu- merged 1 commit into
bluss:masterfrom
kornelski:u16

Conversation

@kornelski

@kornelski kornelski commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

This changes the internal length type from u32 to u16 on 16-bit targets. On 16-bit targets larger arrays won't be used, 32-bit arithmetic can be expensive, and 4 bytes is a significant overhead.

target_pointer_width is not going to be smaller than "16", so #[cfg(not(target_pointer_width = "16"))] is sufficient to mean > 16.

Rustup doesn't ship a 16-bit target by default, so unfortunately it's not easy to add a compile test for it, but the tests pass with u16 LenUint on larger platforms.

@tbu- tbu- left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@tbu-
tbu- merged commit 812c83a into bluss:master Oct 17, 2024
@bluss

bluss commented Oct 17, 2024

Copy link
Copy Markdown
Owner

Is this a breaking change, will need 0.8 or not?

@kornelski

Copy link
Copy Markdown
Contributor Author

It's not a breaking change from the API perspective. The API and external types stay the same, only the internal representation changed, and only on 16-bit targets. Per hyrum's law somebody could be relying on particular values of sizeof or transmute, but I don't think these are promised by semver.

@tbu-

tbu- commented Oct 17, 2024

Copy link
Copy Markdown
Collaborator

This is not a breaking change AFAICT, because capacities greater than u16::MAX cannot be allocated on such machines anyway. This means that there will be a compile-time error for capacities larger than u16::MAX before and after this PR (on 16-bit platforms).

We change the underlying length integer which is not exposed and cannot be observed without mem::transmute or mem::size_of, but these kinds of changes do not count as breaking change (and it would be insane if they did — you couldn't change any private struct member).

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