Skip to content

Avoid ref when using format! in compiler #127980

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jul 20, 2024
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jul 19, 2024

Clean up a few minor refs in format! macro, as it has a performance cost. Apparently the compiler is unable to inline format!("{}", &variable), and does a run-time double-reference instead (format macro already does one level referencing). Inlining format args prevents accidental & misuse.

See also rust-lang/rust-clippy#10851

@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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. labels Jul 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 19, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

@tgross35
Copy link
Contributor

Is there a clippy lint for this? I know it can detect opportunity to put the args inline, but not sure if it flags needless refs here. Seems like it should if this has impact.

@nyurik
Copy link
Contributor Author

nyurik commented Jul 19, 2024

@tgross35 see rust-lang/rust-clippy#10851

@nyurik nyurik closed this Jul 19, 2024
@nyurik nyurik reopened this Jul 19, 2024
@nyurik
Copy link
Contributor Author

nyurik commented Jul 19, 2024

sorry, must have accidentally closed it

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.
@oli-obk
Copy link
Contributor

oli-obk commented Jul 19, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

📌 Commit aef0e34 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2024
@oli-obk oli-obk removed the PG-exploit-mitigations Project group: Exploit mitigations label Jul 19, 2024
@oli-obk oli-obk assigned oli-obk and unassigned fee1-dead Jul 19, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Jul 19, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#123196 (Add Process support for UEFI)
 - rust-lang#127523 (Migrate `dump-ice-to-disk` and `panic-abort-eh_frame` `run-make` tests to rmake)
 - rust-lang#127556 (Replace a long inline "autoref" comment with method docs)
 - rust-lang#127693 (Migrate `crate-hash-rustc-version` to `rmake`)
 - rust-lang#127866 (Conditionally build `wasm-component-ld` )
 - rust-lang#127918 (Safely enforce thread name requirements)
 - rust-lang#127948 (fixes panic error `index out of bounds` in conflicting error)
 - rust-lang#127980 (Avoid ref when using format! in compiler)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#123196 (Add Process support for UEFI)
 - rust-lang#127556 (Replace a long inline "autoref" comment with method docs)
 - rust-lang#127693 (Migrate `crate-hash-rustc-version` to `rmake`)
 - rust-lang#127866 (Conditionally build `wasm-component-ld` )
 - rust-lang#127918 (Safely enforce thread name requirements)
 - rust-lang#127948 (fixes panic error `index out of bounds` in conflicting error)
 - rust-lang#127980 (Avoid ref when using format! in compiler)
 - rust-lang#127984 (Avoid ref when using format! in src)
 - rust-lang#127987 (More accurate suggestion for `-> Box<dyn Trait>` or `-> impl Trait`)

r? `@ghost`
`@rustbot` modify labels: rollup
@@ -759,7 +759,7 @@ fn link_natively(
sess.dcx().abort_if_errors();

// Invoke the system linker
info!("{:?}", &cmd);
info!("{cmd:?}");
Copy link

Choose a reason for hiding this comment

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

Suggested change
info!("{cmd:?}");
info!(?cmd);

Maybe it should be another clippy lint to turn to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not exactly the same, the latter prints cmd = xxzzzzz

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#123196 (Add Process support for UEFI)
 - rust-lang#127556 (Replace a long inline "autoref" comment with method docs)
 - rust-lang#127693 (Migrate `crate-hash-rustc-version` to `rmake`)
 - rust-lang#127866 (Conditionally build `wasm-component-ld` )
 - rust-lang#127918 (Safely enforce thread name requirements)
 - rust-lang#127948 (fixes panic error `index out of bounds` in conflicting error)
 - rust-lang#127980 (Avoid ref when using format! in compiler)
 - rust-lang#127984 (Avoid ref when using format! in src)
 - rust-lang#127987 (More accurate suggestion for `-> Box<dyn Trait>` or `-> impl Trait`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cd8c5f7 into rust-lang:master Jul 20, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2024
Rollup merge of rust-lang#127980 - nyurik:compiler-refs, r=oli-obk

Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
@nyurik nyurik deleted the compiler-refs branch July 20, 2024 15:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

7 participants