-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve structured tuple struct suggestion #88631
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
Conversation
(And same for tuple variants.) Previously, the span was just for the constructor name, which meant it would result in syntactically-invalid code when applied. Now, the span is for the entire expression.
For two reasons: 1. Now that the suggestion span has been corrected, the output is a bit cluttered and hard to read. Putting the suggestion its own window creates more space. 2. It's easier to see what's being suggested, since now the version after the suggestion is applied is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bors r+ |
📌 Commit 2226977 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (d19d864): comparison url. Summary: This change led to small relevant mixed results 🤷 in compiler performance.
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: +S-waiting-on-review -S-waiting-on-perf +perf-regression |
The waiting on review label was added accidentally, since we're now posting perf results after merges happen. I'll remove the label. @rustbot label: -S-waiting-on-review The summary that this PR led to a mixed performance change seems correct though the change is quite small. We might want to tweak the perf bot to not post in such cases. |
The perf results still seem much worse than I would have thought. The only non-error path change is that an extra @rylev Should this be marked as |
Yep - it looks spurious - all the perf changes except 2 are considered "very small" by our standards with the other two barely registering as "small" changes. Given that there doesn't seem to be a plausible explanation, and the perf change is mostly a wash, we should marked this as triaged. @rustbot label: +perf-regression-triaged |
Previously, the span was just for the constructor name, which meant it
would result in syntactically-invalid code when applied. Now, the span
is for the entire expression.
I also changed it to use
span_suggestion_verbose
, for two reasons:Now that the suggestion span has been corrected, the output is a bit
cluttered and hard to read. Putting the suggestion its own window
creates more space.
It's easier to see what's being suggested, since now the version
after the suggestion is applied is shown.
r? @davidtwco