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.

Sign up for GitHub

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

Jump to bottom

Panic directly in Arguments::new* instead of recursing #117804

Merged
merged 1 commit into from
May 23, 2024
+5 −2
Merged

Panic directly in Arguments::new* instead of recursing #117804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions library/core/src/fmt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,9 @@ impl<'a> Arguments<'a> {
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
pub const fn new_const(pieces: &'a [&'static str]) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason it can't be

Suggested change
pub const fn new_const(pieces: &'a [&'static str]) -> Self {
pub const fn new_const(pieces: &'a [&'static str; 1]) -> Self {

eliminating the "runtime" check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function currently is called with references to arrays of length 1 and 0. This is a decent idea, but at least it would require some amount of surgery on the format_args! expansion code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, I overlooked 0 being valid as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about then?

Suggested change
pub const fn new_const(pieces: &'a [&'static str]) -> Self {
pub const fn new_const<const N: usize>(pieces: &'a [&'static str; N]) -> Self {

After monomorphization the check should be eliminat-able this way, though I suspect this function is probably small enough to be probably always inlined anyways, making this point moot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hunh. That's a really good idea.

if pieces.len() > 1 {
panic!("invalid args");
// Since panic!() expands to panic_fmt(format_args!()), using panic! here is both a
// bit silly and also significantly increases the amount of MIR generated by panics.
crate::panicking::panic_nounwind("invalid args");
}
Arguments { pieces, fmt: None, args: &[] }
}
Expand All @@ -350,7 +352,8 @@ impl<'a> Arguments<'a> {
#[inline]
pub fn new_v1(pieces: &'a [&'static str], args: &'a [rt::Argument<'a>]) -> Arguments<'a> {
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
panic!("invalid args");
// See Arguments::new_const for why we don't use panic!.
crate::panicking::panic_nounwind("invalid args");
}
Arguments { pieces, fmt: None, args }
}
Expand Down