Skip to content

Specialize format! for simple "{}" case #76531

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

Closed
wants to merge 3 commits into from
Closed

Specialize format! for simple "{}" case #76531

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0cc6ac
Specialize format! with pattern-matching
workingjubilee Sep 8, 2020
4e45df7
Minimize expansion on NeedsDisplay
workingjubilee Sep 10, 2020
1ee0392
Simplify to invoking Trait on ident
workingjubilee Oct 21, 2020
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
4 changes: 4 additions & 0 deletions library/alloc/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ macro_rules! vec {
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
macro_rules! format {
// A faster path for simple format! calls.
("{}", $arg:ident) => {{
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to make this so restrictive? I think $arg:expr $(,)? would catch a lot more and still allow calling to_string.

Copy link
Contributor

@notriddle notriddle Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

#76531 (comment)

What's worse than that: even the expr version will not be correct.

Right now, format! parses named arguments, like a = 1. If that was parsed as an expression, then it would evaluate to (), and this would not pass the assertion.

let a = format!("{}", x = 1);
assert_eq!(&a, "1");

I think it actually needs to be $arg:ident, and the optimization probably wouldn't be triggered very often as a result. That's annoying.

$crate::fmt::Display::to_string(&$arg)
}};
Copy link
Member

Choose a reason for hiding this comment

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

We will want a codegen test I think for this, at least -- it looks like this is just not triggering to me, because I would expect NeedsDisplay to not resolve here. Generally we need the type to be public in order for it to be constructible outside the current crate.

Copy link
Member Author

@workingjubilee workingjubilee Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

codegen – tests that compile and then test the generated LLVM code to make sure that the optimizations we want are taking effect. See LLVM docs for how to write such tests.

Ah, one of these? Well, that just made this PR more Exciting.

Copy link
Member

Choose a reason for hiding this comment

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

Note that before working on that I would really fix this macro to make sure this arm is getting expanded, because right now that seems pretty unlikely to me. See also the comment below about needing :ident on the arm, most likely, which'll further simplify this branch.

($($arg:tt)*) => {{
let res = $crate::fmt::format($crate::__export::format_args!($($arg)*));
res
Expand Down