-
Notifications
You must be signed in to change notification settings - Fork 13.3k
int audit core::{hash, fmt}, std::sync #22700
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
Conversation
cc #22240 |
I think these are fine yeah, they're not showing up in the public API and we could perhaps add an |
Needs Manishearth@a5009b8 to compile on master. |
fmt and hash are pretty straightforward I think. sync is a bit more complex. I thought one or two of the `isize`s ought to be `i32`s, but that would require a bunch of casting (the root cause being the lack of atomics other than isize/usize). r? @alexcrichton
Also needs b182cd7 Might want to take a look at that (it only touches tests), since your commit message mentions some trickiness in sync. |
let ret = LowerHex::fmt(&(*self as uint), f); | ||
f.flags &= !(1 << (FlagV1::Alternate as uint)); | ||
f.flags |= 1 << (FlagV1::Alternate as u32); | ||
let ret = LowerHex::fmt(&(*self as u32), f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting the pointer to u32
loses part of it on 64bit, see #22854. Should this be usize
instead?
fmt and hash are pretty straightforward I think. sync is a bit more complex. I thought one or two of the
isize
s ought to bei32
s, but that would require a bunch of casting (the root cause being the lack of atomics other than isize/usize).r? @alexcrichton