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

Fix #77 #78

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

Fix #77 #78

wants to merge 10 commits into from

Conversation

noamtashma
Copy link

@noamtashma noamtashma commented Jan 23, 2022

This is my fork that fixes #77 . I based it on the existing work in #72 .

Some things I'm not sure of:

  • Which option for the semantics of OwningRef is appropriate? For now I chose to implement "allow OwningRef::as_owner and similar methods, but disallow convertion from OwningRefMut to OwningRef". But perhaps the other option is better.
  • Should unsafe functions be kept as unsafe (and deprecated) methods for backwards compatibility?
  • How exactly should I change the documentation of these functions?

For now the functions that became unsafe keep their documentation, except it has unsafe in the doc-tests so they wouldn't fail. map_with_owner has changed to be safe, and a new unsafe deprecated function called map_with_owner_direct equivalent to the old one was added, for backwards compatibility. However on second thought creating a new deprecated function doesn't seem like the best decision, so I would like to hear what would be preferred.

And of course, thank you for your time maintaining this library!

steffahn and others added 10 commits January 18, 2021 18:32
for a bit more backwards compatibility
Removed the bad conversion from OwningRefMut to OwningRef.
Adapted some tests, others just added `unsafe`.
Doc tests don't compile.
reference to the owner itself.
Keeping the old version as `.direct` functions, since the
only breakage I could think of
would literally be unsound anyways.
Fixed tests.
@Ten0
Copy link

Ten0 commented Oct 11, 2023

@noamtashma would you consider releasing this as a separate crate on crates.io?

@noamtashma noamtashma force-pushed the fix_unsound_map branch 2 times, most recently from 40bb34f to 3714d36 Compare October 12, 2023 09:58
@noamtashma
Copy link
Author

@Ten0 I released this as the safer_owning_ref crate.

@Ten0
Copy link

Ten0 commented Oct 17, 2023

Amazing, thanks! :)

@Ten0
Copy link

Ten0 commented Oct 23, 2023

Issues are not enabled by default on forks, would it be possible to enable them on the repository so that we can get notified in case somebody notices an issue?

@noamtashma
Copy link
Author

enabled issues

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.

Unsoundness in OwningRef::map_with_owner and more
3 participants