-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Replace Copy/Clone compiler magic on arrays with library impls #86041
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
|
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
The code building the clone shim for array can be removed now as well, right? |
601e975 to
4f2d9b4
Compare
|
I've addressed the feedback and this is ready for review. |
|
☔ The latest upstream changes (presumably #87781) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hi @bstrie! Would you mind rebasing this PR and resolving the merge conflict so we can do a perf run? Assuming that does not reveal any issues, I think we're about ready to merge this. |
jackh726
left a comment
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.
This LGTM to me on compiler and traits changes.
|
Ping from triage: |
|
This seems reasonable to me, once the merge conflicts are resolved and a perf run comes back without regressions. |
4f2d9b4 to
8a772b9
Compare
Imo, i think this would better be left as followup. |
|
Okay, given previous review from @joshtriplett, previous discussion during a compiler meeting, and perf review by @Mark-Simulacrum, and my own review of compiler/traits changes, I'm going to go ahead and @bors r+ |
|
📌 Commit 61b1394 has been approved by |
|
⌛ Testing commit 61b1394 with merge fc59b7e59222dafbb1811b9c34659bf35deff255... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
@bors retry |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (d608229): comparison url. Summary: This change led to moderate 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: +perf-regression |
As of rust-lang/rust#86041, `Clone` impls for array types are now implemented in library code rather than using `clone` shims. As such, the `TyKind::Array` case in the code surrounding `CloneShim`s is now unreachable. This removes the code to simplify things. To prevent future regressions, this adds a test case that `clone`s an array and ensures that it does not emit a `don't know how to build clone shim` warning. Fixes #197.
As of rust-lang/rust#86041, `Clone` impls for array types are now implemented in library code rather than using `clone` shims. As such, the `TyArray` case in the code supporting custom translations for clone shims is now unreachable. This removes the code to simplify things. This also bumps the `mir-json` submodule to bring in a fix for GaloisInc/mir-json#197, which is in similar territory. Note that the already existing `test/conc_eval/array/clone.rs` test case ensures that `crucible-mir` can continue to support cloning arrays without issues. Fixes #1646.
With const generics the compiler no longer needs to fake these impls.