-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
removed unsafe code in favor of explicit assert #32
Conversation
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 A matter of style: I’d prefer writing out Thanks for this contribution. I should once more think about releasing 1.0. |
Rebased and pushed ;) Glad to help! |
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.) |
@chris-morgan This was merged ~1.5 years ago, fixing a UB(you can revert this and run |
@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? |
I don't think that's needed, I think it will be a needless MSRV bump, and both this and |
converting a
*u8
to a*u64
is/could be unsafe, because of different 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