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.

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

Avoid double binding of subdiagnostics inside #[derive(SessionDiagnostic)] #97121

Merged
merged 6 commits into from
May 24, 2022
Merged

Avoid double binding of subdiagnostics inside #[derive(SessionDiagnostic)] #97121

merged 6 commits into from
May 24, 2022

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented May 17, 2022

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 17, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2022
@pvdrz pvdrz changed the title Generate code for subdiagnostic fields in the second match inside #[derive(SessionDiagnostic)] Avoid double binding of subdiagnostics inside #[derive(SessionDiagnostic)] May 18, 2022
@pvdrz pvdrz requested a review from davidtwco May 18, 2022 15:51
pvdrz added 3 commits May 18, 2022 10:53
@pvdrz
move misplaced comment
9521219
@pvdrz
remove unnecessary generics
7e9be92
@pvdrz
let generate_field_attrs_code create FieldInfo
d839d2e 
this simplifies the code inside the `structure.each` closure argument
and allows to remove the `vis` field from `FieldInfo`.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
davidtwco
davidtwco reviewed May 20, 2022
View reviewed changes
compiler/rustc_macros/src/diagnostics/diagnostic.rs
@@ -71,55 +71,42 @@ impl<'a> SessionDiagnosticDerive<'a> {
}
};

// Keep track of which fields are subdiagnostics or have no attributes.
let mut subdiagnostics_or_empty = std::collections::HashSet::new();
Copy link
Member

Choose a reason for hiding this comment

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

I'm torn on whether or not we should build up a set like this or just do the same check again but negate the result (putting it into a function to avoid duplication obviously). Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm under the impression that having the set should be faster than doing the check again but I don't have any hard evidence on it either. I'm happy with both alternatives anyway.

I checked synstructure to see if we could generate both at the same time to avoid the second filter but it seems that's not the case.

@pvdrz pvdrz requested a review from davidtwco May 23, 2022 20:56
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit 8227e69 has been approved by davidtwco

@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 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#97121 - pvdrz:do-subdiagnostics-later, r=da…
c8d4755 
…vidtwco

Avoid double binding of subdiagnostics inside `#[derive(SessionDiagnostic)]`

r? `@davidtwco`
@Dylan-DPC Dylan-DPC mentioned this pull request May 24, 2022
Closed
@bors
Copy link
Contributor

bors commented May 24, 2022

⌛ Testing commit 8227e69 with merge b2eba05...

@bors
Copy link
Contributor

bors commented May 24, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing b2eba05 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 24, 2022
@bors bors merged commit b2eba05 into rust-lang:master May 24, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 24, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b2eba05): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 1 1 2 1
mean2 N/A 2.7% -3.4% -2.2% -3.4%
max N/A 2.7% -3.4% -3.5% -3.4%

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 0 1 1
mean2 2.1% N/A N/A -2.5% 2.1%
max 2.1% N/A N/A -2.5% 2.1%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩ ↩2

  2. the arithmetic mean of the percent change ↩ ↩2

@compiler-errors compiler-errors added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. 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.63.0
Development

Successfully merging this pull request may close these issues.
8 participants
@pvdrz @rust-log-analyzer @davidtwco @bors @rust-timer @compiler-errors @rust-highfive @rustbot