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

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 2 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
16 changes: 16 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 @@ -64,6 +64,15 @@ macro_rules! vec {
($($x:expr,)*) => (vec![$($x),*])
}

// HACK(jubilee): Shim for specializing format! It is possible to manually
// implement ToString, bypassing Display. "{}" normally formats in terms of
// Display. NeedsDisplay enforces equally strict type boundarie.
#[unstable(feature = "display_type_guard", issue = "none")]
#[allow(dead_code)]
struct NeedsDisplay<T: crate::fmt::Display> {
inner: T,
}

/// Creates a `String` using interpolation of runtime expressions.
///
/// The first argument `format!` receives is a format string. This must be a string
Expand Down Expand Up @@ -99,9 +108,16 @@ macro_rules! vec {
/// format!("hello {}", "world!");
/// format!("x = {}, y = {y}", 10, y = 30);
/// ```
#[allow_internal_unstable(display_type_guard)]
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
macro_rules! format {
// A faster path for simple format! calls.
("{}", $($arg:tt,?)+) => {{
Copy link
Member

Choose a reason for hiding this comment

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

nit: does this need to deal with a full $($arg:tt,?)+? Maybe it could just take $e:expr ,? and let all the more complicated cases still be handled by the other arms of the macro.

(I don't think there's a need to optimize format!("{}", 1, 2, 3, 4, 5, 6).)

Copy link
Contributor

@notriddle notriddle Sep 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, I didn't think about that kind of usage.
@notriddle Can you post or link me to some more... uh... weird examples like that? I mean, it might make me actually just close this PR but I'd like to have an opportunity to fool around with the alternate cases to see how I can handle it in the pattern matching and if it destroys the gains.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/std/fmt/index.html#usage

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I'm aware of the usual usages, but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see, I've never seen a named parameter then being passed to an "anonymous" formatting parameter.

Copy link
Member

Choose a reason for hiding this comment

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

but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see

That's because it's a bug and shouldn't be accepted: #45256

let fast = |t: NeedsDisplay<_> | $crate::string::ToString::to_string(t.inner);
// This TokenTree must resolve to a Displayable value to be valid.
fast(NeedsDisplay { inner: &{$($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