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

Shorten def_span of closures to just their header #98482

Merged
merged 6 commits into from
Jul 8, 2022
Merged

Shorten def_span of closures to just their header #98482

merged 6 commits into from
Jul 8, 2022

Conversation

cjgillot
Copy link
Contributor

Continuation of #93967.

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rustbot rustbot added 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 Jun 25, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@cjgillot cjgillot force-pushed the short-struct-span-closure branch from dc11e8f to 4fab055 Compare June 26, 2022 11:14
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the short-struct-span-closure branch from 4fab055 to 985c8a8 Compare June 26, 2022 11:34
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
estebank
estebank reviewed Jun 26, 2022
View reviewed changes
src/test/ui/associated-types/impl-wf-cycle-1.stderr Outdated
Comment on lines 4 to 11
LL | / impl<T: Grault> Grault for (T,)
LL | | where
LL | | Self::A: Baz,
LL | | Self::B: Fiz,
... |
LL | |
LL | | }
| |_^
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, we should probably point at the first where clause in this error as the primary span, but that's out of scope.

estebank
estebank reviewed Jun 26, 2022
View reviewed changes
compiler/rustc_middle/src/hir/map/mod.rs Outdated
span: outer_span,
..
}) => {
let end = if let Some(b) = bounds.last() { b.span() } else { generics.span };
Copy link
Contributor

Choose a reason for hiding this comment

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

Whould we also account for the where clause here when appropriate? (I think that type is the only case where you wouldn't want to due to syntactical ordering.)

estebank
estebank approved these changes Jun 26, 2022
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me after rebasing and optionally addressing the regression in E0596 (or filing a follow up ticket). Make it rollup=never.

@cjgillot cjgillot force-pushed the short-struct-span-closure branch from 55a498f to 4dd3d6d Compare June 27, 2022 19:02
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jun 28, 2022

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

@cjgillot cjgillot force-pushed the short-struct-span-closure branch from 4dd3d6d to 6784137 Compare June 28, 2022 19:55
@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@cjgillot cjgillot force-pushed the short-struct-span-closure branch from 6784137 to 6be9f05 Compare June 28, 2022 19:59
@estebank
Copy link
Contributor

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 28, 2022

📌 Commit 6be9f059cbb3a8ed350ba66b5392c028f875acef has been approved by estebank

@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 Jun 28, 2022
@bors
Copy link
Contributor

bors commented Jun 29, 2022

⌛ Testing commit 6be9f059cbb3a8ed350ba66b5392c028f875acef with merge bd86f533a02ad2bd48343719b946f7dd70a56f28...

@bors
Copy link
Contributor

bors commented Jun 29, 2022

💔 Test failed - checks-actions

@cjgillot cjgillot force-pushed the short-struct-span-closure branch from de0339e to d43fa93 Compare July 7, 2022 07:28
@cjgillot
Copy link
Contributor Author

cjgillot commented Jul 7, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 7, 2022

📌 Commit d43fa93 has been approved by estebank

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 7, 2022
@bors
Copy link
Contributor

bors commented Jul 8, 2022

⌛ Testing commit d43fa93 with merge eba361a...

@bors
Copy link
Contributor

bors commented Jul 8, 2022

☀️ Test successful - checks-actions
Approved by: estebank
Pushing eba361a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2022
@bors bors merged commit eba361a into rust-lang:master Jul 8, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 8, 2022
This was referenced Jul 8, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (eba361a): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.7% 0.7% 1
Regressions 😿
(secondary)		 1.6% 2.4% 9
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 0.7% 0.7% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 1.3% 2.4% 2
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -1.0% -1.0% 1
All 😿🎉 (primary) 1.3% 2.4% 2

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 2.9% 2.9% 1
Regressions 😿
(secondary)		 3.2% 3.7% 5
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.9% 2.9% 1

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

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

Footnotes

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

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Jul 8, 2022
@cjgillot
Copy link
Contributor Author

The regression is mostly in tt-muncher case. From the detailed view, it would be in expand_crate. I don't see how a change in error-reporting span can cause such a performance change.

@cjgillot cjgillot deleted the short-struct-span-closure branch July 11, 2022 09:36
@Mark-Simulacrum Mark-Simulacrum removed the perf-regression Performance regression. label Jul 12, 2022
@Mark-Simulacrum
Copy link
Member

tt-muncher has been noisy lately, so dropping the regression label; this delta is within that noise bound.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
@bors
Auto merge of rust-lang#98482 - cjgillot:short-struct-span-closure, r…
bee9da1 
…=estebank

Shorten def_span of closures to just their header

Continuation of rust-lang#93967.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

Labels
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cjgillot @rust-highfive @rust-log-analyzer @bors @rustbot @estebank @rust-timer @Mark-Simulacrum @flip1995