-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Refactor some std
code that works with pointer offstes
#100823
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
☔ The latest upstream changes (presumably #101152) made this pull request unmergeable. Please resolve the merge conflicts. |
…ls/alloc.rs` nicer - Use `.addr()` instead of `as`-cast - Use `add` instead of `offset` and remove some `as isize` casts by doing that - Remove some casts
ac3455e
to
31b7181
Compare
Rebased to fix merge conflict. r? @scottmcm |
library/core/src/fmt/num.rs
Outdated
let buf_ptr = MaybeUninit::slice_as_mut_ptr(&mut buf); | ||
let lut_ptr = DEC_DIGITS_LUT.as_ptr(); | ||
|
||
// decode 2 chars at a time | ||
while n >= 100 { | ||
let d1 = ((n % 100) as isize) << 1; | ||
let d1 = ((n % 100) << 1) as usize; |
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.
For safety, can you switch this to be more similar to the old one?
let d1 = ((n % 100) << 1) as usize; | |
let d1 = ((n % 100) as usize) << 1; |
The macro defining this function is instantiated with i8
(
rust/library/core/src/fmt/num.rs
Lines 465 to 468 in 3ea4493
impl_Exp!( | |
i8, u8, i16, u16, i32, u32, i64, u64, usize, isize | |
as u64 via to_u64 named exp_u64 | |
); |
n
could be i8
, so if n
was 99_i8
then the shift would overflow if it's done before the cast.
(Can that ever happen? No idea. Up to you if you want to try to write a test to trigger it somehow. But I think it's worth changing the code to obviously not be a problem just in case.)
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.
Oh, wait, no, we only get here if n >= 100
, but (127_i8 % 100) << 1
, so I guess it'd be fine.
That's still a bit too subtle for my taste, though, so I'd rather just shift in the bigger type.
library/core/src/fmt/num.rs
Outdated
} | ||
// `n` < 1e8 < (1 << 32) | ||
let mut n = n as u32; | ||
if n >= 1e4 as u32 { | ||
let to_parse = n % 1e4 as u32; | ||
n /= 1e4 as u32; | ||
|
||
let d1 = (to_parse / 100) << 1; | ||
let d2 = (to_parse % 100) << 1; | ||
let d1 = ((to_parse / 100) << 1) as usize; |
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.
pondering: I'm not sure if moving the casts up to the dN
variables helps. Based on the structure hwere, where these various blocks are working in particular bitwidths based on the range of n
, I think it's kinda nice to have then dN
variables still be in those types. Which would then imply keeping the cast in the pointer operations, just as .add(d1 as usize)
instead of offset
+isize
. How do you feel about that? I'm not sure how strongly I feel here, so absolutely feel free to argue either way 🙃
(The curr
being usize
is definitely a nice change, though.)
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.
I actually agree, dN
are only used once, so it doesn't really matter where the cast is, and with the cast in add
it looks nicer.
library/core/src/fmt/num.rs
Outdated
} else { | ||
let d1 = n << 1; | ||
let d1 = (n << 1) as usize; |
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.
aside: I guess this one is also overflowing when n
is 99_i8
, even before your change, but it just overflows into the sign bit which I guess doesn't matter in the end after the casts?
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.
n is cast to u16
on line 548, so no overflows here :)
Overflow into a sign bit would be pretty bad:
[src/main.rs:2] 99i8 << 1 = -58
[src/main.rs:2] (99i8 << 1) as usize = 18446744073709551558
@@ -316,9 +316,9 @@ where | |||
// | small1 | Chunk smaller than 8 bytes | |||
// +--------+ | |||
fn region_as_aligned_chunks(ptr: *const u8, len: usize) -> (usize, usize, usize) { |
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.
pondering (not this PR): this looks an awful lot like .align_to(8)
.
Looks good! @bors r+ And thanks again for splitting out the various parts of this. |
Refactor some `std` code that works with pointer offstes This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it. This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine). r? ``@scottmcm`` _split off from rust-lang#100746, continuation of #100822_
Refactor some `std` code that works with pointer offstes This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it. This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine). r? ```@scottmcm``` _split off from rust-lang#100746, continuation of #100822_
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#100823 (Refactor some `std` code that works with pointer offstes) - rust-lang#102088 (Fix wrongly refactored Lift impl) - rust-lang#102109 (resolve: Set effective visibilities for imports more precisely) - rust-lang#102186 (Add const_closure, Constify Try trait) - rust-lang#102203 (rustdoc: remove no-op CSS `#source-sidebar { z-index }`) - rust-lang#102204 (Make `ManuallyDrop` satisfy `~const Destruct`) - rust-lang#102210 (diagnostics: avoid syntactically invalid suggestion in if conditionals) - rust-lang#102226 (bootstrap/miri: switch to non-deprecated env var for setting the sysroot folder) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This PR replaces
pointer::offset
in standard library withpointer::add
andpointer::sub
, [re]moving some casts and using.addr()
while we are at it.This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃 (though I've checked this multiple times and it looks fine).
r? @scottmcm
split off from #100746, continuation of #100822