-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq #105750
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. Please see the contribution instructions for more information. |
I can add the rest of the valtree work to this PR if preferred, but I thought I'd submit it in this small form first. |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit ed0627422a3a653595718616513ce5b8a8aab8c6 with merge 73bd817a44e4fa3751ed828e99297caf55470e77... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (73bd817a44e4fa3751ed828e99297caf55470e77): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
Blocked on making all function pointers PartialEq, even ones with higher ranked lifetimes, |
So, basically #99531. It's not impossible but it seems hard. If I understand correctly, this PR still uses the old "destructure const" machinery to build the pattern tree. Is the idea that after this PR, we can change that to use valtrees (and remove const destructuring) without further behavior changes? |
Yes, I have tried this locally and it works and will permit more cleanups |
Still blocked on #99531 |
@rustbot author we got a |
@bors r- |
This comment has been minimized.
This comment has been minimized.
@bors r=lcnr |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9239760): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 641.949s -> 642.522s (0.09%) |
thir::pattern: update some comments and error type names Follow-up to [these comments](rust-lang#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
thir::pattern: update some comments and error type names Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
thir::pattern: update some comments and error type names Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
thir::pattern: update some comments and error type names Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
Right now we destructure the constant as far as we can, but with this PR we just don't take it apart anymore. This is preparatory work for moving to always using valtrees, as these will just do a single conversion of the constant to a valtree at the start, and if that fails, fall back to
PartialEq
.This removes a few cases where we emitted the
unreachable pattern
lint, because we stop looking into the constant deeply enough to detect that a constant is already covered by another pattern.Previous work: #70743
This is groundwork towards fixing #83085 and #105047