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

Use edition 2018, reduce unsafe code, replace deprecated uninitialized with MaybeUnit #138

Closed
wants to merge 4 commits into from
Closed

Conversation

WildCryptoFox
Copy link

I was trying to add #![forbid(unsafe_code)] but couldn't figure out how to eliminate all the unsafe { core::mem::uninitialized() }.

I know these are all ZSTs and eliminating these calls should not help with anything; but I'd like to see them gone. Any ideas?

In the meantime I have updated typenum to 2018 edition, reduced a little of the unsafe code and replaced the deprecated uninitialized()s with MaybeUninit::zeroed().assume_init().

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Jan 17, 2020

As demonstrated with the CI test failure. This does increase the minimum rustc version requirement to 1.31.

Correction: 1.31 was necessary for 2018 edition, but now we need 1.36. Alternatively we can dynamically check the version and decide which method for uninitialization we want. The current stable is 1.40.

Of the top-10 (non-transitive) reverse-dependencies of typenum the only two with a commitment for rustc version are generic-array and uom with 1.30.1 and 1.31 respectively.

@paholg
Copy link
Owner

paholg commented Apr 1, 2020

Hey, sorry I have been unresponsive. I did not have the mental energy to look at this repo for a while there.

Thank you for the PR. A more recent PR, #142, was able to get rid of unsafe entirely, so I'm inclined to go with it.

I would also like to keep from bumping the minimum rust version if possible. The last time I (accidentally) bumped it, I almost immediately had an issue for it (#122).

@jerry73204
Copy link
Contributor

@WildCryptoFox I had a re-work on my fork (diff link). How could we merge our work?

@WildCryptoFox
Copy link
Author

@jerry73204 For a start you've mangled my commits and lost the signatures. Please reduce the commits under your name and state only that it was derived from my commits. Add fixes #138 in the git commit message to close this automatically when merged.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Jul 25, 2020

Sorry @paholg I seem to have missed your comment in April. Glad to see the newer PR removed the unsafe code. :)

You will still need to upgrade to at least 1.31 but with the newer PR removing the unsafe code; if that didn't push the version up then you may not need any newer than 1.31. You need 1.31 to use edition = "2018".

jerry73204 pushed a commit to jerry73204/typenum that referenced this pull request Jul 25, 2020
@jerry73204
Copy link
Contributor

@WildCryptoFox I would respect your work and keep your author name on the commits, while mine is on the "commit" author. Perhaps you can rebase your branch on mine to finish this PR?

The versions and commit msgs are changed accordingly now.

@WildCryptoFox
Copy link
Author

@jerry73204 I appreciate you wanting to preserve my attribution; but in this case any original code worth attribution has been erased through the process of updating it due to the accepted PR which removed the unsafe code. The remaining changes consist only of the trivial and standard procedure for upgrading from edition 2015 to 2018; and these do not deserve my attribution. If I were to rebase this PR, I'd only repeat the original process (ignoring the old commits) or repeat the work you've already done. Please flatten the commits to remove my name and commit messages; then just cite derivation from this PR. Thank you.

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