Implementation of fmt::FormattingOptions

[EliasHolzmann]

Tracking issue: #118117

Public API:

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct FormattingOptions { … }
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Sign {
    Plus, 
    Minus
}
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum DebugAsHex {
    Lower,
    Upper
}

impl FormattingOptions {
    pub fn new() -> Self;
    pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self;
    pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self;
    pub fn alternate(&mut self, alternate: bool) -> &mut Self;
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self;
    pub fn width(&mut self, width: Option<usize>) -> &mut Self;
    pub fn precision(&mut self, precision: Option<usize>) -> &mut Self;
    pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self;

    pub fn get_sign(&self) -> Option<Sign>;
    pub fn get_sign_aware_zero_pad(&self) -> bool;
    pub fn get_alternate(&self) -> bool;
    pub fn get_fill(&self) -> char;
    pub fn get_align(&self) -> Option<Alignment>;
    pub fn get_width(&self) -> Option<usize>;
    pub fn get_precision(&self) -> Option<usize>;
    pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>;

    pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>;
    }

impl<'a> Formatter<'a> {
    pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self;
    pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>;
    pub fn sign(&self) -> Option<Sign>;

    pub fn options(&self) -> FormattingOptions;
}

Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing #[derive(..)]s and pub specifiers):

• enum DebugAsHex/FormattingOptions::debug_as_hex/FormattingOptions::get_debug_as_hex: To support {:x?} as well as {:X?}. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.
• fill/get_fill now takes/returns char instead of Option<char>. This simply mirrors what Formatter::fill returns (with default being ' ').
• Changed zero_pad/get_zero_pad to sign_aware_zero_pad/get_sign_aware_zero_pad. This also mirrors Formatter::sign_aware_zero_pad. While I'm not a fan of this quite verbose name, I do believe that having the interface of Formatter and FormattingOptions be compatible is more important.
• For the same reason, renamed alignment/get_alignment to aling/get_align.
• Deviating from my initial idea, Formatter::with_options returns a Formatter which has the lifetime of the self reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned Formatter was the generic lifetime used by self instead). Otherwise, one could construct two Formatters that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using Formatter::new, so I believe it is worthwhile to keep this method.

[rustbot]

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

[EliasHolzmann]

Looks like this broke some compiler tests. Will fix this evening, converting to draft PR for now.

[EliasHolzmann]

CI is green -> Converting back to "normal" (non-draft) PR.

[thomcc]

I already have a pretty big review backlog and haven't been following this change (so I'd have to read the ACP to figure out how it's supposed to behave), so I'm going to reroll for now, sorry.

r? libs

[EliasHolzmann]

Fixed two more issues I've stumbled onto while playing around with the PR changes (see commit descriptions for details). I'll fix the PR description to represent the current API.

[bors]

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

[m-ou-se]

enum DebugAsHex/FormattingOptions::debug_as_hex/FormattingOptions::get_debug_as_hex: To support {:x?} as well as {:X?}. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.

We haven't decided yet what to do with the "debug as hex" flags. It's fine to have them as unstable, but we shouldn't stabilize an interface for those flags without a separate discussion. I've added it as an unresolved question to the tracking issue.

[m-ou-se]

This makes a Formatter object bigger than before, because the flags field is now stored in the new FormattingOptions as separate fields.

We should check if that doesn't have any negative impact on performance.

[EliasHolzmann]

This makes a Formatter object bigger than before

Are you sure? Before, flags was a u32, so 4 bytes. With each flag in its own field, there are two bools and two small enums wrapped into Option – both should be one byte per field, so 4 bytes in total as well. Unless I'm not seeing something here, both the original code and the new implementation have the same size.

However, the benchmark shows a performance regression, and when I realized that Formatter didn't get bigger and therefore, flags may not be the culprit, I was already halfway through refactoring FormattingOptions to use a bitmask for flags (like it did before). So, either way, let's test that, it might help even if a difference in size is not the cause of the performance regression. Could you please rerun the benchmark?

[m-ou-se]

Are you sure?

No :)

Before, flags was a u32, so 4 bytes. With each flag in its own field, there are two bools and two small enums wrapped into Option – both should be one byte per field, so 4 bytes in total as well.

Good point!

I think that in the near future we might want to put more things in the fields flag though. At least the alignment can easiliy fit in there, and the Some/None flag of the width and precision could fit in there as well:

pub struct FormattingOptions {
    flags: u32, // sign, zero pad, alt, debug-as-hex, width flag, precision flag, alignment
    fill: char,
    width: usize, // only used if width flag is set.
    precision: usize, // only used if width flag is set.
}

But maybe that's overkill.

[programmerjake]

pub struct FormattingOptions {
    flags: u32, // sign, zero pad, alt, debug-as-hex, width flag, precision flag, alignment
    fill: char,
    width: usize, // only used if width flag is set.
    precision: usize, // only used if width flag is set.
}

well, fill has lots of spare bits, you could probably merge it with flags, where flags is just the upper bits and fill is the lower 21 bits. though this would only be beneficial on <= 32-bit systems unless width and precision were changed to be u32 or the struct was #[repr(packed(4))].

[EliasHolzmann]

I agree that there is definitely potential for further optimization here. I'll leave the solution using flags – while it is a bit harder to read, the possible performance benefits in the future seem more important to me.

The benchmark still reports a small performance regression (~0.1 %). While I don't think this is a big problem, it would still be nice if this PR would not introduce any performance regressions. To make the code more maintainable, I changed all internal accesses to formatting options to use the getters/setters on FormattingOptions instead of direct field access. I suspect this is at least a major factor in the performance regression. I have reverted this now, can you please rerun the benchmark @m-ou-se?

[m-ou-se]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 385944a with merge 5085172...

[bors]

☀️ Try build successful - checks-actions
Build commit: 5085172 (508517284df836989dd91522841c917d69eac778)

[apiraino]

#135672 was accepted regression in our beta crater run so I think it's correct to label this for the release notes

@rustbot label +relnotes

[nyurik]

It seems there is a bug with fill -- new() sets it to a ' ' (space), but FormattingOptions also uses #[derive(Default)] which will set fill to \0. I don't think this was intended, was it? Seems the Default trait impl should use new()?

P.S. I created a fix in #135977