-
Notifications
You must be signed in to change notification settings - Fork 13k
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
make find_similar_impl_candidates
even fuzzier
#93298
Conversation
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
Marking as blocked on #92223. |
☔ The latest upstream changes (presumably #93348) made this pull request unmergeable. Please resolve the merge conflicts. |
baa625a
to
fe076e2
Compare
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.
I like the simplification of the logic. However, I feel the compiler now emits too many low-relevance suggestions, for instance integer types. Is there a way to filter them out?
ty::Adt(adt, ..) => match adt.adt_kind() { | ||
AdtKind::Struct => Some(15), | ||
AdtKind::Union => Some(16), | ||
AdtKind::Enum => Some(17), |
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.
Why merge struct/enum/union categories?
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.
because for adts we always compare their AdtDef
anyways, so giving the different kinds different categories doesn't matter
|
||
None | ||
}) | ||
.collect() |
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.
Do we need to collect, or can we return an impl Iterator
?
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.
would need rpitit for this, as find_similar_impl_candidates
is defined on InferCtxt
using an extension trait
☔ The latest upstream changes (presumably #92007) made this pull request unmergeable. Please resolve the merge conflicts. |
0b96439
to
8b31c73
Compare
not 100% happy with which impls are suggested here, but efficiently fixing that probably needs a more general rework of this code which i don't have the capacity for rn |
This comment has been minimized.
This comment has been minimized.
So let's go with this first step. |
☔ The latest upstream changes (presumably #93893) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r=cjgillot rollup |
📌 Commit f2aea1e has been approved by |
⌛ Testing commit f2aea1e with merge e9ec070cc26027bcae2069663baaa1b027407c1b... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
seems spurious @bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (52dd59e): comparison url. Summary: This benchmark run shows 2 relevant improvements 🎉 but 6 relevant regressions 😿 to instruction counts.
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 |
how does this change perf? :o bit confused by the results. keccak check, full has a 0.78% regression but its detailed data shows that it is 2.0% faster? https://perf.rust-lang.org/detailed-query.html?commit=52dd59ed2154f4158ae37e6994b678a6249a7bb0&base_commit=b321742c6c27494897a88cd5ac17ac20aa3469a1&benchmark=keccak-check&scenario=full Maybe |
continues the good work of @BGR360 in #92223. I might have overshot a bit and we're now slightly too fuzzy 😅
with this we can now also simplify
simplify_type
, which is nice :3