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.

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

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

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.

compiler/rustc_driver_impl/src/lib.rs Show resolved Hide resolved
compiler/rustc_expand/src/mbe/diagnostics.rs Show resolved Hide resolved
@WaffleLapkin WaffleLapkin self-assigned this May 16, 2023
@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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