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

Provide zero-cost Optioned<char> #19

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Provide zero-cost Optioned<char> #19

merged 1 commit into from
Jul 26, 2017

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jul 21, 2017

This uses char::from_u32_unsafe(u32::MAX) as the none value. This is not a valid char value, and will never be a valid Unicode code point. Because of this, it is possible to provide a zero cost Optioned<char> by assigning the Noned value to this invalid code point.

So long as Optioned holds its contract of not giving out the noned value, this can in no way be memory unsafe because the fake char is only used as a sentinel, and never as an actual character.

This uses `char::from_u32_unsafe(u32::MAX)` as the none value. This is not a valid `char` value, and will never be a valid Unicode code point. Because of this, it is possible to provide a zero cost `Optioned<char>` by assigning the `Noned` value to this invalid code point.

So long as `Optioned` holds its contract of not giving out the noned value, this can in no way be memory unsafe because the fake char is only used as a sentinel, and never as an actual character.
@CAD97
Copy link
Contributor Author

CAD97 commented Jul 21, 2017

Failure is not mine; it's #20.

@llogiq
Copy link
Owner

llogiq commented Jul 21, 2017

Hey, thanks for this! Unfortunately, both my operating systems broke down recently, so I'm still in the process of setting up and cannot test this over the weekend. I hope I can get around to it next Monday. I'm sorry to let you wait, but I'd really like to test it before merging.

@llogiq llogiq merged commit c6129cb into llogiq:master Jul 26, 2017
@llogiq
Copy link
Owner

llogiq commented Jul 26, 2017

Thanks again, merged. Will publish shortly.

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