Skip to content

inline format!() args up to and including rustc_middle (2) #114068

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
Merged
merged 1 commit into from
Jul 30, 2023
Merged

inline format!() args up to and including rustc_middle (2) #114068

merged 1 commit into from
Jul 30, 2023

Conversation

matthiaskrgr
Copy link
Member

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jul 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@matthiaskrgr matthiaskrgr changed the title inline formt!() args up to and including rustc_middle inline format!() args up to and including rustc_middle Jul 25, 2023
@matthiaskrgr matthiaskrgr changed the title inline format!() args up to and including rustc_middle inline format!() args up to and including rustc_middle (2) Jul 25, 2023
WaffleLapkin
WaffleLapkin requested changes Jul 26, 2023
View reviewed changes
compiler/rustc_builtin_macros/src/asm.rs Outdated
Comment on lines 683 to 686
help_str.push_str(&format!(" {{{sym}}}"));
"named argument never used"
} else {
help_str.push_str(&format!(" {{{}}}", idx));
help_str.push_str(&format!(" {{{idx}}}"));
Copy link
Member

Choose a reason for hiding this comment

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

Those two are probably best reverted (so as to not to be confused with {{idx}})

compiler/rustc_builtin_macros/src/format_foreign.rs Outdated
Comment on lines 644 to 645
Substitution::Ordinal(n, _) => Ok(format!("{{{n}}}")),
Substitution::Name(n, _) => Ok(format!("{{{n}}}")),
Copy link
Member

Choose a reason for hiding this comment

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

Same deal here, revert those please.

compiler/rustc_expand/src/mbe/macro_parser.rs Outdated
@@ -736,7 +736,7 @@ impl TtParser {
"local ambiguity when calling macro `{}`: multiple parsing options: {}",
self.macro_name,
match self.next_mps.len() {
0 => format!("built-in NTs {}.", nts),
0 => format!("built-in NTs {nts}."),
n => format!("built-in NTs {} or {n} other option{s}.", nts, s = pluralize!(n)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
n => format!("built-in NTs {} or {n} other option{s}.", nts, s = pluralize!(n)),
n => format!("built-in NTs {nts} or {n} other option{s}.", s = pluralize!(n)),

@WaffleLapkin
Copy link
Member

Also, just a note: @matthiaskrgr please, don't include tags (@username) in commits (I get a notification every time you force push).

@matthiaskrgr
Copy link
Member Author

Oh crap, sorry, I guess I can force-push them out and that would resolve the issue?

@WaffleLapkin
Copy link
Member

It should resolve the issue, yes.

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2023
@bors
Copy link
Collaborator

bors commented Jul 27, 2023

☔ The latest upstream changes (presumably #113281) made this pull request unmergeable. Please resolve the merge conflicts.

@WaffleLapkin WaffleLapkin mentioned this pull request Jul 29, 2023
Closed
@matthiaskrgr
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 30, 2023
WaffleLapkin
WaffleLapkin approved these changes Jul 30, 2023
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

r=me with commit message typo fixed

formt!()

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2023
@matthiaskrgr
Copy link
Member Author

😅
@bors r=WaffleLapkin

@bors
Copy link
Collaborator

bors commented Jul 30, 2023

📌 Commit 2381546 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 30, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 30, 2023
@NobodyXu
Copy link
Contributor

I'm not familiar with rustc, but I wonder if inlining format! args would allow it to replace the formatting with a constant string when all arguments are known at compile-time, like format_args! inline optimization.

@matthiaskrgr matthiaskrgr mentioned this pull request Jul 30, 2023
Merged
@workingjubilee
Copy link
Member

  1. hypothetically
  2. be careful not to confuse inlining, the optimization, with implicit named argument capture in format_args!, which is sometimes referred to as "inline format_args!"

@WaffleLapkin
Copy link
Member

@NobodyXu format!("blah {}", x) → format!("blah {x}") is purely syntactic transformation, the semantics don't change. It neither helps nor makes it harder to do optimizations.

Moreover format! also uses format_args! under the hood, so optimizations which inline constants also benefit it.

@NobodyXu
Copy link
Contributor

Thank you for the explanation!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2023
@bors
Auto merge of rust-lang#114250 - matthiaskrgr:rollup-0r0dhrr, r=matth…
89acdae 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110056 (Fix the example in document for WaitTimeoutResult::timed_out)
 - rust-lang#112655 (Mark `map_or` as `#[must_use]`)
 - rust-lang#114018 (Make `--error-format human-annotate-rs` handle multiple files)
 - rust-lang#114068 (inline format!() args up to and including rustc_middle (2))
 - rust-lang#114223 (Documentation: Fix Stilted Language in Vec->Indexing)
 - rust-lang#114227 (Add tidy check for stray rustfix files)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a964f94 into rust-lang:master Jul 30, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 30, 2023
@lcnr lcnr removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Aug 4, 2023
@matthiaskrgr matthiaskrgr deleted the fmt_args_rustc_1 branch January 25, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

Assignees

@WaffleLapkin WaffleLapkin

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
7 participants
@matthiaskrgr @rustbot @WaffleLapkin @bors @NobodyXu @workingjubilee @lcnr