Skip to content

Avoid &format("...") calls in error message code. #111633

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 2 commits into from
May 18, 2023

Conversation

nnethercote
Copy link
Contributor

Some error message cleanups. Best reviewed one commit at a time.

r? @davidtwco

@rustbot rustbot added 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 16, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-log-analyzer

This comment has been minimized.

Error message all end up passing into a function as an `impl
Into<{D,Subd}iagnosticMessage>`. If an error message is creatd as
`&format("...")` that means we allocate a string (in the `format!`
call), then take a reference, and then clone (allocating again) the
reference to produce the `{D,Subd}iagnosticMessage`, which is silly.

This commit removes the leading `&` from a lot of these cases. This
means the original `String` is moved into the
`{D,Subd}iagnosticMessage`, avoiding the double allocations. This
requires changing some function argument types from `&str` to `String`
(when all arguments are `String`) or `impl
Into<{D,Subd}iagnosticMessage>` (when some arguments are `String` and
some are `&str`).
@est31
Copy link
Member

est31 commented May 16, 2023

I'm not sure this is a good idea as this error code is to be migrated away from, and some crates have migration PRs open for a while already, and this PR might cause conflicts for them.

Edit: talking about the second commit. The first commit is fine.

@nnethercote
Copy link
Contributor Author

I'm not sure this is a good idea as this error code is to be migrated away from, and some crates have migration PRs open for a while already, and this PR might cause conflicts for them.

@davidtwco approved a bunch of similar changes in #110579. The changes are all very simple, so any conflicts should be easy to resolve.

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.

A couple .as_str()s that I don't really understand, but otherwise LGTM.

@WaffleLapkin WaffleLapkin self-assigned this May 16, 2023
@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 17, 2023

📌 Commit 01e33a3 has been approved by WaffleLapkin

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 May 17, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#110884 (Support RISC-V unaligned-scalar-mem target feature)
 - rust-lang#111160 (Update serde in workspace and non-synced dependencies)
 - rust-lang#111168 (Specialize ToString implementation for fmt::Arguments)
 - rust-lang#111527 (add examples of port 0 binding behavior)
 - rust-lang#111561 (Include better context for "already exists" error in compiletest)
 - rust-lang#111633 (Avoid `&format("...")` calls in error message code.)
 - rust-lang#111679 (Remove libs message about ACPs from triagebot)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 08efb9d into rust-lang:master May 18, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 18, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 23, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 23, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jun 15, 2023
…ffleLapkin

Avoid `&format("...")` calls in error message code.

Some error message cleanups. Best reviewed one commit at a time.

r? `@davidtwco`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2023
…sage-code, r=oli-obk

Avoid `&format` in error message code

follow-up of rust-lang#111633
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2023
…sage-code, r=oli-obk

Avoid `&format` in error message code

follow-up of rust-lang#111633
# 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. T-rustdoc Relevant to the rustdoc 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