dtolnay / ryu

Fast floating point to string conversion
Apache License 2.0
606 stars 27 forks source link

Add MaybeUninit support #16

Closed anna-is-cute closed 5 years ago

anna-is-cute commented 5 years ago

Hello!

This PR adds use of MaybeUninit everywhere that mem::uninitialized was previously used.

The first commit does it in a breaking way, and the second commit only does it on Rust 1.36+, where MaybeUninit is stable, falling back on mem::uninitialized. I'll squash them, but I find it more readable to be able to see them separately for review purposes.

I'm not sure if I've done it 100% correctly, so I'd appreciate a review. It did require a large helping of #[cfg], but I'm not sure how else to handle it.

Tests all pass and benchmarks appear unaffected.

I have a few concerns:

Thanks!

(Edit: Sorry for all the pushes. 1.15.0 won't update the registry on my laptop at home for some reason.)

anna-is-cute commented 5 years ago

Okay, all CI builds and tests pass now. :) The 1.15.0 warning about use core::ptr; being unused is fun. It stops being unused in later Rusts (for example, at least by 1.35.0), so I'm not sure what to do about that guy.

anna-is-cute commented 5 years ago

Both!

  1. mem::uninitialized is deprecated and will eventually become a removed lang item, causing ryu to no longer compile, so this change will have to be made eventually, anyway.
  2. The docs for MaybeUninit state the following:

    The compiler, in general, assumes that variables are properly initialized at their respective type. For example, a variable of reference type must be aligned and non-NULL. This is an invariant that must always be upheld, even in unsafe code.

    Moreover, uninitialized memory is special in that the compiler knows that it does not have a fixed value. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type, which otherwise can hold any fixed bit pattern:

    (Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

    As I understand it from the above, it is UB to even initialise integer types using mem::uninitialized, which creating an array does.

dtolnay commented 5 years ago

and will eventually become a removed lang item

Ah I didn't know about this. Could you share a link to where you found this?

bjorn3 commented 5 years ago

Ah I didn't know about this. Could you share a link to where you found this?

It was attempted in rust-lang/rust#62150, and then backed out in https://github.com/rust-lang/rust/pull/63343, as the new mem::uninitialized implementation was causing SIGILL.

anna-is-cute commented 5 years ago

It was attempted in rust-lang/rust#62150, and then backed out in rust-lang/rust#63343, as the new mem::uninitialized implementation was causing SIGILL.

Those implement mem::uninitialized in terms of MaybeUninit, but they don't remove the deprecation. I could swear I read explicitly that mem::uninitialized would be removed, but maybe not. The function is deprecated, and that is the idea behind deprecating, though, is it not?

dtolnay commented 5 years ago

No, in general we don't remove previously stable things from the standard library. Deprecation means there is something else you should probably use instead for new code, but existing code needs to continue to compile. Deprecation does not mean something is scheduled for removal.

dtolnay commented 5 years ago

Published in 1.0.1. :beers: