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

minor fixes to remove 'deprecated' warnings #26

Merged
merged 1 commit into from
Nov 21, 2020
Merged

minor fixes to remove 'deprecated' warnings #26

merged 1 commit into from
Nov 21, 2020

Conversation

JOE1994
Copy link
Contributor

@JOE1994 JOE1994 commented Aug 14, 2020

Hello 🦀 , this PR makes the following changes

  • add dyn keyword for type declaration of trait objects
    (Using trait objects without a dyn keyword is deprecated)
  • minor refactorings to make code shorter

Thank you for reviewing this PR 👍

closes #25

@zslayton
Copy link
Owner

Thanks for opening this PR! The changeset looks good to me, but the CI build is failing because of the library's use of mem::uninitialized. I have an experimental branch that should remove that; I'll merge this after that's fixed.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Aug 19, 2020

Thank you for your feedback! I should've run cargo test before submitting this PR! my bad.. 😿

I see the tests failed due to the assertion check inside mem::uninitialized().
Simply replacing mem::uninitialized() to MaybeUninit::uninit().assume_init() seems to be a good enough fix to me,
and I made another commit making that change.

If you were thinking of doing something else in your branch to fix this issue, I'll get rid of the new commit with a force-push! Thank you 🦀

@zslayton
Copy link
Owner

Simply replacing mem::uninitialized() to MaybeUninit::uninit().assume_init() seems to be a good enough fix to me,
and I made another commit making that change.

I believe that this fixes the compiler error but not the underlying problem. Check out the safety warning in the assume_init method's documentation:

It is up to the caller to guarantee that the MaybeUninit really is in an initialized state. Calling this when the content is not yet fully initialized causes immediate undefined behavior.

Using uninit().assume_init() is labeled as 'an incorrect usage' just below that in the examples:

use std::mem::MaybeUninit;

let x = MaybeUninit::<Vec<u32>>::uninit();
let x_init = unsafe { x.assume_init() };
// `x` had not been initialized yet, so this last line caused undefined behavior. ⚠️

To be clear, lifeguard already had this problem because it was using mem::uninitialized, and I appreciate your effort in trying to resolve it! But I don't think this is the fix we need.

In my experimental branch, I was using the unsafe ptr::read function to move the value out of the RecycledInner in the detach method and then immediately calling mem::forget on the RecycledInner. This created a clone of the value that could be returned to the user and guaranteed that Drop would not be called for the original value. This achieves the same thing as the current code but doesn't rely on uninitialized memory to work.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Aug 19, 2020

I thought MaybeUninit::uninit().assume_init() would be fine since the program never reads from the uninitialized memory.
(I could be wrong)

Using ptr::read sounds better, since the code wouldn't be using uninitialized memory at all 👍
I'll make a force-push to undo the second commit!

@zslayton
Copy link
Owner

Sorry this took so long to get merged, @JOE1994. Thanks again for contributing!

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.

Heads-up: UB due to misuse of mem::uninitialized will soon lead to panic
2 participants