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

Change null handling when creating Ids #131

Merged
merged 2 commits into from
Mar 2, 2022
Merged

Change null handling when creating Ids #131

merged 2 commits into from
Mar 2, 2022

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Feb 28, 2022

Option::unwrap_unchecked is stable since 1.58, and that is IMO much clearer than wrapping things in NonNull beforehand (see the changelog entry for an example).

The real motivation for this change is from #81, where the naive implementation of Id::retain_autoreleased_null would have been completely broken (the extra branch in NonNull::new(ptr).map(|ptr| unsafe { Id::retain_autoreleased(ptr) }) would interfere).

Also, looking through the code I seem to have used NonNull::new_unchecked a lot more than what's entirely correct (+ alloc/+ new can return nil when out of memory), so changing those to unwrap makes things better in that regard (albeit a tiny bit slower).

@madsmtm madsmtm added bug Something isn't working enhancement New feature or request labels Feb 28, 2022
@madsmtm madsmtm changed the title Change null handling when creating Ids Change null handling when creating Ids Feb 28, 2022
@madsmtm madsmtm force-pushed the id-null-handling branch 6 times, most recently from eb4ea99 to 08ae307 Compare March 2, 2022 19:17
@madsmtm madsmtm merged commit 0e868ca into master Mar 2, 2022
@madsmtm madsmtm deleted the id-null-handling branch March 2, 2022 20:34
@madsmtm madsmtm added this to the objc2 v0.3 milestone Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant