Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madhav-madhusoodanan
Copy link
Contributor

@madhav-madhusoodanan madhav-madhusoodanan commented Mar 11, 2025

ACP approval: rust-lang/libs-team#546 (comment)

Context

  1. Implemented integer formatting into a fixed-size buffer without relying on fmt.
  2. Added a NumBuffer type to help with this process.

Associated Issue

r? @hanna-kruppe

@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2025

Failed to set assignee to hanna-kruppe: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 11, 2025
@madhav-madhusoodanan
Copy link
Contributor Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@madhav-madhusoodanan madhav-madhusoodanan marked this pull request as ready for review March 18, 2025 15:00
@madhav-madhusoodanan
Copy link
Contributor Author

I was able to gain more clarity on what was needed, and I've moved the updates to the fmt module.

Primarily I've broken down impl_Display into impl_NumBuffer (the primitives) and impl_Display so that I can reuse functionality into the new macro impl_FormatInto.

Sorry for the long history in this PR. Please let me know if this looks good.

@madhav-madhusoodanan madhav-madhusoodanan changed the title [WIP] Functionality for int_format_into for integer types Functionality for int_format_into for integer types Mar 18, 2025
Copy link
Contributor

@hanna-kruppe hanna-kruppe left a 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.

@madhav-madhusoodanan
Copy link
Contributor Author

madhav-madhusoodanan commented Mar 18, 2025

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.

I must thank you for patiently going through my updates multiple times. There was a lot for me to learn.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

I'm going to re-roll to get someone hopefully more familiar with the formatting stuff.
r? libs

@rustbot rustbot assigned thomcc and unassigned scottmcm Mar 20, 2025
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 22, 2025

☔ The latest upstream changes (presumably #136974) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 24, 2025
@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 24, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

/// `MaybeUninit<u8>`.
#[unstable(feature = "int_format_into", issue = "138215")]
#[derive(Debug)]
pub struct NumBuffer {
Copy link
Member

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);
    }
}

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants