removed unsafe code in favor of explicit assert

[hellow554]

converting a *u8 to a *u64 is/could be unsafe, because of different alignment.

 casting from `*const u8` to a more-strictly-aligned pointer (`*const u64`)                                                                                                       
   --> src/raw.rs:28:38                                                                                                                                                                  
      |
   28 |             ptr::copy_nonoverlapping(&bytes[0] as *const u8 as *const u64, &mut self.value, 1)                                                                                     
      |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                          
      |                                                                                                                                                                                    
   = note: #[deny(clippy::cast_ptr_alignment)] on by default                                                                                                                            
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#cast_ptr_alignment    

This PR will have no runtime effect, because assert! influences the code generated by rust/llvm, so they are equal, but less unsafe :)

https://rust.godbolt.org/z/FXkwB2

[chris-morgan]

Ah, I love clever compilers. I love clever people, too. ☺

Thanks for going to the trouble of confirming the compiled result. I see from this that simply changing the debug_assert! to assert! would probably have been helpful anyway—subtly changing the panic behaviour, to a failed assertion rather than a failed bounds check, as it is thereby able to omit the bounds check. Then the instructions emitted truly are identical to your not-unsafe version.

A matter of style: I’d prefer writing out u64::from(…) eight times instead of going to the trouble of defining a shorthand u(). It’s still short enough that it doesn’t mess up line lengths or make things too obscure or anything, so I’d prefer it that way.

Thanks for this contribution. I should once more think about releasing 1.0.

[hellow554]

Rebased and pushed ;) Glad to help!

[chris-morgan]

BTW, the explanation and justification you put in the PR description would make a good commit message. I encourage you to get in the habit of writing long commit messages, if you’re not already, typically with explanations as to why things changed, just as you would normally need to to get changes accepted into an open source project. That sort of thing is quite often very useful later on. Embrace the long commit messages!

(I try to convince people of this often; in my day job at FastMail I hold nine or ten of the top ten longest commit messages—some may think I take it too far; but I find it occurs surprisingly often that when I’m defending my work in a commit message, or even explaining what I did, I realise why the solution won’t work in certain cases, or is inferior to another.)

[elichai]

@chris-morgan This was merged ~1.5 years ago, fixing a UB(you can revert this and run cargo +nightly miri test to see the unaligned copy). Can you please publish a new version? There's 21 crates depending on this crate, some have tens of thousands of downloads.

[hellow554]

@elichai in the meanwhile "from_be_bytes" was stabilized and would be a better solution for this.

https://doc.rust-lang.org/stable/std/primitive.u64.html#method.from_be_bytes

Would you like to open a PR and fix this?

[elichai]

I don't think that's needed, I think it will be a needless MSRV bump, and both this and from_be_bytes will be optimized exactly the same