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

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Loading status checks…
panic_nounwind in Arguments::new* instead of recursing
@saethlin
saethlin committed May 23, 2024

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
commit 75f3cef756afa4ae5d8b9c9e6b52124701c2285b
7 changes: 5 additions & 2 deletions library/core/src/fmt/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -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: &[] }
}
@@ -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 }
}