-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Functionality for int_format_into
for integer types
#138338
base: master
Are you sure you want to change the base?
Functionality for int_format_into
for integer types
#138338
Conversation
Failed to set assignee to
|
@rustbot author |
This comment has been minimized.
This comment has been minimized.
d247480
to
dcb7e5d
Compare
This comment has been minimized.
This comment has been minimized.
dcb7e5d
to
cdd0823
Compare
This comment has been minimized.
This comment has been minimized.
cdd0823
to
a8968ea
Compare
This comment has been minimized.
This comment has been minimized.
a8968ea
to
e1d3241
Compare
This comment has been minimized.
This comment has been minimized.
e1d3241
to
ab8dd05
Compare
This comment has been minimized.
This comment has been minimized.
ab8dd05
to
890aaf1
Compare
This comment has been minimized.
This comment has been minimized.
890aaf1
to
8a6b99b
Compare
This comment has been minimized.
This comment has been minimized.
8a6b99b
to
0b924f5
Compare
This comment has been minimized.
This comment has been minimized.
0b924f5
to
4d0187e
Compare
This comment has been minimized.
This comment has been minimized.
4d0187e
to
4a67fb1
Compare
This comment has been minimized.
This comment has been minimized.
4a67fb1
to
2c1eba8
Compare
This comment has been minimized.
This comment has been minimized.
2c1eba8
to
d5834e9
Compare
I was able to gain more clarity on what was needed, and I've moved the updates to the Primarily I've broken down Sorry for the long history in this PR. Please let me know if this looks good. |
int_format_into
for integer typesint_format_into
for integer types
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.
Thank you for working on this! The bird's eye view of the diff is now basically what I'd expect. I left some smaller comments I've noticed while scrolling through. I don't expect I'll have the time to review the implementation details as carefully as unsafe code deserves, so please work out those details with a reviewer who actually has permissions on this repository.
177df1d
to
e57b53c
Compare
I must thank you for patiently going through my updates multiple times. There was a lot for me to learn. |
This comment has been minimized.
This comment has been minimized.
e57b53c
to
d887240
Compare
This comment has been minimized.
This comment has been minimized.
I'm going to re-roll to get someone hopefully more familiar with the formatting stuff. |
d887240
to
2e18654
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #136974) made this pull request unmergeable. Please resolve the merge conflicts. |
2e18654
to
0d8caf0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0d8caf0
to
b60f35a
Compare
This comment has been minimized.
This comment has been minimized.
b60f35a
to
447d72c
Compare
This comment has been minimized.
This comment has been minimized.
447d72c
to
21fda7d
Compare
/// `MaybeUninit<u8>`. | ||
#[unstable(feature = "int_format_into", issue = "138215")] | ||
#[derive(Debug)] | ||
pub struct NumBuffer { |
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.
as mentioned on the ACP and listed as an unresolved issue, I think NumBuffer
should have a generic parameter for the type of integer being formatted (we could extend this to support floats later), so when we add future types like u8192
you don't have to have a huge buffer for u32
even though u8192
needs a huge buffer.
so like:
#[unstable(...)] // impl details
/// Safety: `Buffer` must be `[MaybeUninit<u8>; N]` where `N` is big enough to fit any value of `Self` when formatted.
pub unsafe trait NumBufferTrait {
type Buffer: AsMut<[MaybeUninit<u8>]> + AsRef<[MaybeUninit<u8>]> + 'static + Sync + Send;
const UNINIT_BUFFER: Self::Buffer;
}
// impl NumBufferTrait for u8, i8, u16, i16, ...
pub struct NumBuffer<T: NumBufferTrait> {
contents: T::Buffer,
}
impl<T: NumBufferTrait> NumBuffer<T> {
pub const fn new() -> Self {
Self { contents: T::UNINIT_BUFFER }
}
pub fn len(&self) -> usize {
self.contents.as_ref().len()
}
pub(crate) unsafe fn extract<I>(&self, index: I) -> &I::Output
where
I: SliceIndex<[MaybeUninit<u8>]>,
{
// SAFETY: `index` is bound-checked by the caller.
unsafe { self.contents.as_ref().get_unchecked(index) }
}
#[cfg(feature = "optimize_for_size")]
pub(crate) fn extract_start_mut_ptr(buf: &mut Self) -> *mut u8 {
MaybeUninit::slice_as_mut_ptr(buf.contents.as_mut())
}
pub(crate) unsafe fn write(&mut self, offset: usize, data: u8) {
self.contents.as_mut()[offset].write(data);
}
}
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 think this can be done in a follow up PR.
ACP approval: rust-lang/libs-team#546 (comment)
Context
fmt
.NumBuffer
type to help with this process.Associated Issue
r? @hanna-kruppe