-
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
Reduce formatting width
and precision
to 16 bits
#136932
base: master
Are you sure you want to change the base?
Changes from all commits
1fbb7ec
ee6d703
b61458a
c81180b
1fe3f1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -294,8 +294,8 @@ pub struct FormattingOptions { | |
flags: u32, | ||
fill: char, | ||
align: Option<Alignment>, | ||
width: Option<usize>, | ||
precision: Option<usize>, | ||
width: Option<u16>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pondering: is (I guess that's not really this PR's problem, though.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A width of 0 can be different than a no width. (It behaves the same for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, in the next PR (#136974), it becomes a regular |
||
precision: Option<u16>, | ||
} | ||
|
||
impl FormattingOptions { | ||
|
@@ -389,7 +389,7 @@ impl FormattingOptions { | |
/// the padding specified by [`FormattingOptions::fill`]/[`FormattingOptions::align`] | ||
/// will be used to take up the required space. | ||
#[unstable(feature = "formatting_options", issue = "118117")] | ||
pub fn width(&mut self, width: Option<usize>) -> &mut Self { | ||
pub fn width(&mut self, width: Option<u16>) -> &mut Self { | ||
self.width = width; | ||
self | ||
} | ||
|
@@ -403,7 +403,7 @@ impl FormattingOptions { | |
/// - For floating-point types, this indicates how many digits after the | ||
/// decimal point should be printed. | ||
#[unstable(feature = "formatting_options", issue = "118117")] | ||
pub fn precision(&mut self, precision: Option<usize>) -> &mut Self { | ||
pub fn precision(&mut self, precision: Option<u16>) -> &mut Self { | ||
self.precision = precision; | ||
self | ||
} | ||
|
@@ -455,12 +455,12 @@ impl FormattingOptions { | |
} | ||
/// Returns the current width. | ||
#[unstable(feature = "formatting_options", issue = "118117")] | ||
pub const fn get_width(&self) -> Option<usize> { | ||
pub const fn get_width(&self) -> Option<u16> { | ||
self.width | ||
} | ||
/// Returns the current precision. | ||
#[unstable(feature = "formatting_options", issue = "118117")] | ||
pub const fn get_precision(&self) -> Option<usize> { | ||
pub const fn get_precision(&self) -> Option<u16> { | ||
self.precision | ||
} | ||
/// Returns the current precision. | ||
|
@@ -1499,15 +1499,18 @@ unsafe fn run(fmt: &mut Formatter<'_>, arg: &rt::Placeholder, args: &[rt::Argume | |
unsafe { value.fmt(fmt) } | ||
} | ||
|
||
unsafe fn getcount(args: &[rt::Argument<'_>], cnt: &rt::Count) -> Option<usize> { | ||
unsafe fn getcount(args: &[rt::Argument<'_>], cnt: &rt::Count) -> Option<u16> { | ||
match *cnt { | ||
#[cfg(bootstrap)] | ||
rt::Count::Is(n) => Some(n as u16), | ||
#[cfg(not(bootstrap))] | ||
rt::Count::Is(n) => Some(n), | ||
rt::Count::Implied => None, | ||
rt::Count::Param(i) => { | ||
debug_assert!(i < args.len()); | ||
// SAFETY: cnt and args come from the same Arguments, | ||
// which guarantees this index is always within bounds. | ||
unsafe { args.get_unchecked(i).as_usize() } | ||
unsafe { args.get_unchecked(i).as_u16() } | ||
} | ||
} | ||
} | ||
|
@@ -1516,11 +1519,11 @@ unsafe fn getcount(args: &[rt::Argument<'_>], cnt: &rt::Count) -> Option<usize> | |
#[must_use = "don't forget to write the post padding"] | ||
pub(crate) struct PostPadding { | ||
fill: char, | ||
padding: usize, | ||
padding: u16, | ||
} | ||
|
||
impl PostPadding { | ||
fn new(fill: char, padding: usize) -> PostPadding { | ||
fn new(fill: char, padding: u16) -> PostPadding { | ||
PostPadding { fill, padding } | ||
} | ||
|
||
|
@@ -1634,7 +1637,7 @@ impl<'a> Formatter<'a> { | |
} | ||
// Check if we're over the minimum width, if so then we can also | ||
// just write the bytes. | ||
Some(min) if width >= min => { | ||
Some(min) if width >= usize::from(min) => { | ||
write_prefix(self, sign, prefix)?; | ||
self.buf.write_str(buf) | ||
} | ||
|
@@ -1645,7 +1648,7 @@ impl<'a> Formatter<'a> { | |
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)?; | ||
let post_padding = self.padding(min - width as u16, Alignment::Right)?; | ||
self.buf.write_str(buf)?; | ||
post_padding.write(self)?; | ||
self.options.fill = old_fill; | ||
|
@@ -1654,7 +1657,7 @@ impl<'a> Formatter<'a> { | |
} | ||
// Otherwise, the sign and prefix goes after the padding | ||
Some(min) => { | ||
let post_padding = self.padding(min - width, Alignment::Right)?; | ||
let post_padding = self.padding(min - width as u16, Alignment::Right)?; | ||
write_prefix(self, sign, prefix)?; | ||
self.buf.write_str(buf)?; | ||
post_padding.write(self) | ||
|
@@ -1703,7 +1706,7 @@ impl<'a> Formatter<'a> { | |
// If our string is longer that the precision, then we must have | ||
// truncation. However other flags like `fill`, `width` and `align` | ||
// must act as always. | ||
if let Some((i, _)) = s.char_indices().nth(max) { | ||
if let Some((i, _)) = s.char_indices().nth(usize::from(max)) { | ||
// LLVM here can't prove that `..i` won't panic `&s[..i]`, but | ||
// we know that it can't panic. Use `get` + `unwrap_or` to avoid | ||
// `unsafe` and otherwise don't emit any panic-related code | ||
|
@@ -1724,14 +1727,14 @@ impl<'a> Formatter<'a> { | |
let chars_count = s.chars().count(); | ||
// If we're under the maximum width, check if we're over the minimum | ||
// width, if so it's as easy as just emitting the string. | ||
if chars_count >= width { | ||
if chars_count >= usize::from(width) { | ||
self.buf.write_str(s) | ||
} | ||
// If we're under both the maximum and the minimum width, then fill | ||
// up the minimum width with the specified string + some alignment. | ||
else { | ||
let align = Alignment::Left; | ||
let post_padding = self.padding(width - chars_count, align)?; | ||
let post_padding = self.padding(width - chars_count as u16, align)?; | ||
self.buf.write_str(s)?; | ||
post_padding.write(self) | ||
} | ||
|
@@ -1745,7 +1748,7 @@ impl<'a> Formatter<'a> { | |
/// thing that is being padded. | ||
pub(crate) fn padding( | ||
&mut self, | ||
padding: usize, | ||
padding: u16, | ||
default: Alignment, | ||
) -> result::Result<PostPadding, Error> { | ||
let align = self.align().unwrap_or(default); | ||
|
@@ -1785,19 +1788,19 @@ impl<'a> Formatter<'a> { | |
|
||
// remove the sign from the formatted parts | ||
formatted.sign = ""; | ||
width = width.saturating_sub(sign.len()); | ||
width = width.saturating_sub(sign.len() as u16); | ||
self.options.fill = '0'; | ||
self.options.align = Some(Alignment::Right); | ||
} | ||
|
||
// remaining parts go through the ordinary padding process. | ||
let len = formatted.len(); | ||
let ret = if width <= len { | ||
let ret = if usize::from(width) <= len { | ||
// no padding | ||
// SAFETY: Per the precondition. | ||
unsafe { self.write_formatted_parts(&formatted) } | ||
} else { | ||
let post_padding = self.padding(width - len, Alignment::Right)?; | ||
let post_padding = self.padding(width - len as u16, Alignment::Right)?; | ||
// SAFETY: Per the precondition. | ||
unsafe { | ||
self.write_formatted_parts(&formatted)?; | ||
|
@@ -2029,7 +2032,7 @@ impl<'a> Formatter<'a> { | |
#[must_use] | ||
#[stable(feature = "fmt_flags", since = "1.5.0")] | ||
pub fn width(&self) -> Option<usize> { | ||
self.options.width | ||
self.options.width.map(|x| x as usize) | ||
} | ||
|
||
/// Returns the optionally specified precision for numeric types. | ||
|
@@ -2060,7 +2063,7 @@ impl<'a> Formatter<'a> { | |
#[must_use] | ||
#[stable(feature = "fmt_flags", since = "1.5.0")] | ||
pub fn precision(&self) -> Option<usize> { | ||
self.options.precision | ||
self.options.precision.map(|x| x as usize) | ||
} | ||
|
||
/// Determines if the `+` flag was specified. | ||
|
@@ -2800,7 +2803,7 @@ pub(crate) fn pointer_fmt_inner(ptr_addr: usize, f: &mut Formatter<'_>) -> Resul | |
f.options.flags |= 1 << (rt::Flag::SignAwareZeroPad as u32); | ||
|
||
if f.options.width.is_none() { | ||
f.options.width = Some((usize::BITS / 4) as usize + 2); | ||
f.options.width = Some((usize::BITS / 4) as u16 + 2); | ||
} | ||
} | ||
f.options.flags |= 1 << (rt::Flag::Alternate as u32); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,11 @@ pub enum Alignment { | |
#[derive(Copy, Clone)] | ||
pub enum Count { | ||
/// Specified with a literal number, stores the value | ||
#[cfg(bootstrap)] | ||
Is(usize), | ||
/// Specified with a literal number, stores the value | ||
#[cfg(not(bootstrap))] | ||
Is(u16), | ||
/// Specified using `$` and `*` syntaxes, stores the index into `args` | ||
Param(usize), | ||
/// Not specified | ||
|
@@ -74,7 +78,7 @@ enum ArgumentType<'a> { | |
formatter: unsafe fn(NonNull<()>, &mut Formatter<'_>) -> Result, | ||
_lifetime: PhantomData<&'a ()>, | ||
}, | ||
Count(usize), | ||
Count(u16), | ||
} | ||
|
||
/// This struct represents a generic "argument" which is taken by format_args!(). | ||
|
@@ -150,8 +154,12 @@ impl Argument<'_> { | |
Self::new(x, UpperExp::fmt) | ||
} | ||
#[inline] | ||
#[track_caller] | ||
pub const fn from_usize(x: &usize) -> Argument<'_> { | ||
Argument { ty: ArgumentType::Count(*x) } | ||
if *x > u16::MAX as usize { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should special-case |
||
panic!("Formatting argument out of range"); | ||
m-ou-se marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
Argument { ty: ArgumentType::Count(*x as u16) } | ||
} | ||
|
||
/// Format this placeholder argument. | ||
|
@@ -181,7 +189,7 @@ impl Argument<'_> { | |
} | ||
|
||
#[inline] | ||
pub(super) const fn as_usize(&self) -> Option<usize> { | ||
pub(super) const fn as_u16(&self) -> Option<u16> { | ||
match self.ty { | ||
ArgumentType::Count(count) => Some(count), | ||
ArgumentType::Placeholder { .. } => None, | ||
|
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.
It looks like
expr_usize
,expr_u32
, andexpr_u16
are all nearly identical.Now that there's three copies, can you dedup their implementations to share code? Maybe
expr_unsigned
that takes aast::UintTy
or something?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.
The difference is that they take a
u16
oru32
as argument. I think that's a useful distinction.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 don't mean to delete the type-specific methods, just to have them share an implementation.
So it'd be something like
etc, rather than repeating all the arena alloc and
LitKind
and such in all three of them