From 6090bb48c881617a993c08feb7e9ecdb8ffcc37f Mon Sep 17 00:00:00 2001 From: joboet Date: Tue, 11 Feb 2025 12:24:24 +0100 Subject: [PATCH 1/2] core: use `FormattingOptions` by reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of the time, the options passed to `Formatter` will be the unmodified default options. The current approach of using the options by-value means that a new `FormattingOptions` structure has to be created every time a formatter is constructed. With this PR, `Formatter` stores the options by-reference, which means that one `FormattingOptions` instance in static memory is enough to create a `Formatter`. In the future (and in particular once #119899 has merged), we might even consider removing `fmt::rt::Placeholder` in favour of `FormattingOptions`, which would – in combination with this change – decrease the option-copying even further. --- library/alloc/src/string.rs | 2 +- library/core/src/fmt/mod.rs | 117 ++++++++++++++++++----------------- library/std/src/panicking.rs | 2 +- 3 files changed, 63 insertions(+), 58 deletions(-) diff --git a/library/alloc/src/string.rs b/library/alloc/src/string.rs index b29f740ef0f2a..d120dfeb1e1ab 100644 --- a/library/alloc/src/string.rs +++ b/library/alloc/src/string.rs @@ -2751,7 +2751,7 @@ impl SpecToString for T { default fn spec_to_string(&self) -> String { let mut buf = String::new(); let mut formatter = - core::fmt::Formatter::new(&mut buf, core::fmt::FormattingOptions::new()); + core::fmt::Formatter::new(&mut buf, const { &core::fmt::FormattingOptions::new() }); // Bypass format_args!() to avoid write_str with zero-length strs fmt::Display::fmt(self, &mut formatter) .expect("a Display implementation returned an error unexpectedly"); diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index a1bf3a4d7a706..bdc9ea5433afe 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -482,7 +482,7 @@ impl FormattingOptions { /// /// You may alternatively use [`Formatter::new()`]. #[unstable(feature = "formatting_options", issue = "118117")] - pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a> { + pub fn create_formatter<'a>(&'a self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a> { Formatter { options: self, buf: write } } @@ -530,9 +530,8 @@ impl Default for FormattingOptions { #[stable(feature = "rust1", since = "1.0.0")] #[rustc_diagnostic_item = "Formatter"] pub struct Formatter<'a> { - options: FormattingOptions, - buf: &'a mut (dyn Write + 'a), + options: &'a FormattingOptions, } impl<'a> Formatter<'a> { @@ -544,14 +543,21 @@ impl<'a> Formatter<'a> { /// /// You may alternatively use [`FormattingOptions::create_formatter()`]. #[unstable(feature = "formatting_options", issue = "118117")] - pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self { - Formatter { options, buf: write } + pub fn new(write: &'a mut (dyn Write + 'a), options: &'a FormattingOptions) -> Self { + Formatter { buf: write, options } + } + + fn reborrow(&mut self) -> Formatter<'_> { + Formatter { buf: self.buf, options: self.options } } /// Creates a new formatter based on this one with given [`FormattingOptions`]. #[unstable(feature = "formatting_options", issue = "118117")] - pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b> { - Formatter { options, buf: self.buf } + pub fn with_options<'f, 'o>(&'f mut self, options: &'o FormattingOptions) -> Formatter<'f> + where + 'o: 'f, + { + Formatter { buf: self.buf, options } } } @@ -1429,7 +1435,7 @@ pub trait UpperExp { /// [`write!`]: crate::write! #[stable(feature = "rust1", since = "1.0.0")] pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result { - let mut formatter = Formatter::new(output, FormattingOptions::new()); + let mut formatter = Formatter::new(output, const { &FormattingOptions::new() }); let mut idx = 0; match args.fmt { @@ -1478,15 +1484,17 @@ pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result { } unsafe fn run(fmt: &mut Formatter<'_>, arg: &rt::Placeholder, args: &[rt::Argument<'_>]) -> Result { - fmt.options.fill = arg.fill; - fmt.options.align = arg.align.into(); - fmt.options.flags = arg.flags; - // SAFETY: arg and args come from the same Arguments, - // which guarantees the indexes are always within bounds. - unsafe { - fmt.options.width = getcount(args, &arg.width); - fmt.options.precision = getcount(args, &arg.precision); - } + let options = FormattingOptions { + flags: arg.flags, + fill: arg.fill, + align: arg.align.into(), + // SAFETY: arg and args come from the same Arguments, + // which guarantees the indexes are always within bounds. + width: unsafe { getcount(args, &arg.width) }, + // SAFETY: likewise. + precision: unsafe { getcount(args, &arg.precision) }, + }; + let mut fmt = fmt.with_options(&options); // Extract the correct argument debug_assert!(arg.position < args.len()); @@ -1496,7 +1504,7 @@ unsafe fn run(fmt: &mut Formatter<'_>, arg: &rt::Placeholder, args: &[rt::Argume // Then actually do some printing // SAFETY: this is a placeholder argument. - unsafe { value.fmt(fmt) } + unsafe { value.fmt(&mut fmt) } } unsafe fn getcount(args: &[rt::Argument<'_>], cnt: &rt::Count) -> Option { @@ -1641,16 +1649,16 @@ impl<'a> Formatter<'a> { // The sign and prefix goes before the padding if the fill character // is zero Some(min) if self.sign_aware_zero_pad() => { - let old_fill = crate::mem::replace(&mut self.options.fill, '0'); - let old_align = - crate::mem::replace(&mut self.options.align, Some(Alignment::Right)); - write_prefix(self, sign, prefix)?; - let post_padding = self.padding(min - width, Alignment::Right)?; - self.buf.write_str(buf)?; - post_padding.write(self)?; - self.options.fill = old_fill; - self.options.align = old_align; - Ok(()) + let options = FormattingOptions { + fill: '0', + align: Some(Alignment::Right), + ..self.options().clone() + }; + let mut f = self.with_options(&options); + write_prefix(&mut f, sign, prefix)?; + let post_padding = f.padding(min - width, Alignment::Right)?; + f.write_str(buf)?; + post_padding.write(&mut f) } // Otherwise, the sign and prefix goes after the padding Some(min) => { @@ -1776,9 +1784,9 @@ impl<'a> Formatter<'a> { // for the sign-aware zero padding, we render the sign first and // behave as if we had no sign from the beginning. let mut formatted = formatted.clone(); - let old_fill = self.options.fill; - let old_align = self.options.align; - if self.sign_aware_zero_pad() { + + let options; + let mut f = if self.sign_aware_zero_pad() { // a sign always goes first let sign = formatted.sign; self.buf.write_str(sign)?; @@ -1786,27 +1794,30 @@ impl<'a> Formatter<'a> { // remove the sign from the formatted parts formatted.sign = ""; width = width.saturating_sub(sign.len()); - self.options.fill = '0'; - self.options.align = Some(Alignment::Right); - } + options = FormattingOptions { + fill: '0', + align: Some(Alignment::Right), + ..self.options().clone() + }; + self.with_options(&options) + } else { + self.reborrow() + }; // remaining parts go through the ordinary padding process. let len = formatted.len(); - let ret = if width <= len { + if width <= len { // no padding // SAFETY: Per the precondition. - unsafe { self.write_formatted_parts(&formatted) } + unsafe { f.write_formatted_parts(&formatted) } } else { - let post_padding = self.padding(width - len, Alignment::Right)?; + let post_padding = f.padding(width - len, Alignment::Right)?; // SAFETY: Per the precondition. unsafe { - self.write_formatted_parts(&formatted)?; + f.write_formatted_parts(&formatted)?; } - post_padding.write(self) - }; - self.options.fill = old_fill; - self.options.align = old_align; - ret + post_padding.write(&mut f) + } } else { // this is the common case and we take a shortcut // SAFETY: Per the precondition. @@ -2610,7 +2621,7 @@ impl<'a> Formatter<'a> { /// Returns the formatting options this formatter corresponds to. #[unstable(feature = "formatting_options", issue = "118117")] - pub const fn options(&self) -> FormattingOptions { + pub const fn options(&self) -> &FormattingOptions { self.options } } @@ -2789,28 +2800,22 @@ impl Pointer for *const T { /// /// [problematic]: https://github.com/rust-lang/rust/issues/95489 pub(crate) fn pointer_fmt_inner(ptr_addr: usize, f: &mut Formatter<'_>) -> Result { - let old_width = f.options.width; - let old_flags = f.options.flags; + let mut options = f.options().clone(); // The alternate flag is already treated by LowerHex as being special- // it denotes whether to prefix with 0x. We use it to work out whether // or not to zero extend, and then unconditionally set it to get the // prefix. if f.alternate() { - f.options.flags |= 1 << (rt::Flag::SignAwareZeroPad as u32); + options.flags |= 1 << (rt::Flag::SignAwareZeroPad as u32); - if f.options.width.is_none() { - f.options.width = Some((usize::BITS / 4) as usize + 2); + if options.width.is_none() { + options.width = Some((usize::BITS / 4) as usize + 2); } } - f.options.flags |= 1 << (rt::Flag::Alternate as u32); - - let ret = LowerHex::fmt(&ptr_addr, f); - - f.options.width = old_width; - f.options.flags = old_flags; + options.flags |= 1 << (rt::Flag::Alternate as u32); - ret + LowerHex::fmt(&ptr_addr, &mut f.with_options(&options)) } #[stable(feature = "rust1", since = "1.0.0")] diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 8e50bf11dd082..a6c6944d0e277 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -637,7 +637,7 @@ pub fn begin_panic_handler(info: &core::panic::PanicInfo<'_>) -> ! { // Lazily, the first time this gets called, run the actual string formatting. self.string.get_or_insert_with(|| { let mut s = String::new(); - let mut fmt = fmt::Formatter::new(&mut s, fmt::FormattingOptions::new()); + let mut fmt = fmt::Formatter::new(&mut s, const { &fmt::FormattingOptions::new() }); let _err = fmt::Display::fmt(&inner, &mut fmt); s }) From cffe20d61e634000746023ac22b3a7cc25bfc8a7 Mon Sep 17 00:00:00 2001 From: joboet Date: Tue, 11 Feb 2025 13:08:29 +0100 Subject: [PATCH 2/2] bless codegen test --- ....float_to_exponential_common.GVN.panic-abort.diff | 12 ++++++++++-- ...float_to_exponential_common.GVN.panic-unwind.diff | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-abort.diff b/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-abort.diff index a1be927e1c04a..175caf938a27b 100644 --- a/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-abort.diff +++ b/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-abort.diff @@ -29,6 +29,7 @@ debug precision => _8; let _8: usize; scope 5 (inlined Formatter::<'_>::precision) { + let mut _23: &std::fmt::FormattingOptions; } } } @@ -36,17 +37,21 @@ scope 4 (inlined Formatter::<'_>::sign_plus) { let mut _20: u32; let mut _21: u32; + let mut _22: &std::fmt::FormattingOptions; } bb0: { StorageLive(_4); + StorageLive(_22); StorageLive(_20); StorageLive(_21); - _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32); + _22 = copy ((*_1).1: &std::fmt::FormattingOptions); + _21 = copy ((*_22).0: u32); _20 = BitAnd(move _21, const 1_u32); StorageDead(_21); _4 = Ne(move _20, const 0_u32); StorageDead(_20); + StorageDead(_22); StorageLive(_5); switchInt(copy _4) -> [0: bb2, otherwise: bb1]; } @@ -65,7 +70,10 @@ bb3: { StorageLive(_6); - _6 = copy (((*_1).0: std::fmt::FormattingOptions).4: std::option::Option); + StorageLive(_23); + _23 = copy ((*_1).1: &std::fmt::FormattingOptions); + _6 = copy ((*_23).4: std::option::Option); + StorageDead(_23); _7 = discriminant(_6); switchInt(move _7) -> [1: bb4, 0: bb6, otherwise: bb9]; } diff --git a/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-unwind.diff b/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-unwind.diff index 87ab71feb2faa..af1bf86511620 100644 --- a/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-unwind.diff +++ b/tests/mir-opt/funky_arms.float_to_exponential_common.GVN.panic-unwind.diff @@ -29,6 +29,7 @@ debug precision => _8; let _8: usize; scope 5 (inlined Formatter::<'_>::precision) { + let mut _23: &std::fmt::FormattingOptions; } } } @@ -36,17 +37,21 @@ scope 4 (inlined Formatter::<'_>::sign_plus) { let mut _20: u32; let mut _21: u32; + let mut _22: &std::fmt::FormattingOptions; } bb0: { StorageLive(_4); + StorageLive(_22); StorageLive(_20); StorageLive(_21); - _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32); + _22 = copy ((*_1).1: &std::fmt::FormattingOptions); + _21 = copy ((*_22).0: u32); _20 = BitAnd(move _21, const 1_u32); StorageDead(_21); _4 = Ne(move _20, const 0_u32); StorageDead(_20); + StorageDead(_22); StorageLive(_5); switchInt(copy _4) -> [0: bb2, otherwise: bb1]; } @@ -65,7 +70,10 @@ bb3: { StorageLive(_6); - _6 = copy (((*_1).0: std::fmt::FormattingOptions).4: std::option::Option); + StorageLive(_23); + _23 = copy ((*_1).1: &std::fmt::FormattingOptions); + _6 = copy ((*_23).4: std::option::Option); + StorageDead(_23); _7 = discriminant(_6); switchInt(move _7) -> [1: bb4, 0: bb6, otherwise: bb9]; }