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

Use Cow in {D,Subd}iagnosticMessage. #111748

Merged
merged 1 commit into from
May 29, 2023

Conversation

nnethercote
Copy link
Contributor

Each of {D,Subd}iagnosticMessage::{Str,Eager} has a comment:

// FIXME(davidtwco): can a `Cow<'static, str>` be used here?

This commit answers that question in the affirmative. It's not the most compelling change ever, but it might be worth merging.

This requires changing the impl<'a> From<&'a str> impls to impl From<&'static str>, which involves a bunch of knock-on changes that require/result in call sites being a little more precise about exactly what kind of string they use to create errors, and not just &str. This will result in fewer unnecessary allocations, though this will not have any notable perf effects given that these are error paths.

Note that I was lazy within Clippy, using to_string in a few places to preserve the existing string imprecision. I could have used impl Into<{D,Subd}iagnosticMessage> in various places as is done in the compiler, but that would have required changes to many call sites (mostly changing &format("...") to format!("...")) which didn't seem worthwhile.

r? @WaffleLapkin

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

rustbot commented May 19, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

rustc_error_messages was changed

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member

Do I understand it right, that since we need to store diagnostics, having &'not_static str and String are equivalent (because we convert former into latter)?

(i.e. I want to make sure we don't exclude anything by requiring &'static str)

@nnethercote
Copy link
Contributor Author

Do I understand it right, that since we need to store diagnostics, having &'not_static str and String are equivalent (because we convert former into latter)?

I guess you could say that. I.e. currently we convert any &str into a String in the From impl by allocating. This PR means we avoid doing that most of the time when the error message is a &'static str.

@bors
Copy link
Contributor

bors commented May 20, 2023

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

@nnethercote
Copy link
Contributor Author

I have rebased and addressed @RalfJung's comment.

@rust-log-analyzer

This comment has been minimized.

src/tools/clippy/clippy_utils/src/diagnostics.rs Outdated Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

@WaffleLapkin: what's the status here, are we good to proceed?

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.

I'm not entirely sure this is a good idea, since some call cites got marginally more complex, but it's probably fine.

r=me with the nit

Each of `{D,Subd}iagnosticMessage::{Str,Eager}` has a comment:
```
// FIXME(davidtwco): can a `Cow<'static, str>` be used here?
```
This commit answers that question in the affirmative. It's not the most
compelling change ever, but it might be worth merging.

This requires changing the `impl<'a> From<&'a str>` impls to `impl
From<&'static str>`, which involves a bunch of knock-on changes that
require/result in call sites being a little more precise about exactly
what kind of string they use to create errors, and not just `&str`. This
will result in fewer unnecessary allocations, though this will not have
any notable perf effects given that these are error paths.

Note that I was lazy within Clippy, using `to_string` in a few places to
preserve the existing string imprecision. I could have used `impl
Into<{D,Subd}iagnosticMessage>` in various places as is done in the
compiler, but that would have required changes to *many* call sites
(mostly changing `&format("...")` to `format!("...")`) which didn't seem
worthwhile.
@nnethercote
Copy link
Contributor Author

@bors r=WaffleLapkin

@bors
Copy link
Contributor

bors commented May 28, 2023

📌 Commit 781111e has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 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 May 28, 2023
@bors
Copy link
Contributor

bors commented May 29, 2023

⌛ Testing commit 781111e with merge 70e04bd...

@bors
Copy link
Contributor

bors commented May 29, 2023

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 70e04bd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2023
@bors bors merged commit 70e04bd into rust-lang:master May 29, 2023
@rustbot rustbot added this to the 1.72.0 milestone May 29, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (70e04bd): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.7% [0.6%, 0.8%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-0.8%, -0.3%] 10
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.1% [2.9%, 6.6%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 642.999s -> 643.87s (0.14%)

@rustbot rustbot added the perf-regression Performance regression. label May 29, 2023
@nnethercote nnethercote deleted the Cow-DiagnosticMessage branch May 30, 2023 09:02
@nnethercote
Copy link
Contributor Author

Perf changes are all for secondary benchmarks. The small wins and small losses balance out.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label May 30, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request May 31, 2023
smoelius added a commit to trailofbits/dylint that referenced this pull request Jun 1, 2023
floriangru added a commit to floriangru/creusot that referenced this pull request Jul 6, 2023
lifetimes for those slices changed to 'static
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.

9 participants