-
Notifications
You must be signed in to change notification settings - Fork 13.7k
introduce the Comparable trait for BTree operations #144506
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
r? @ibraheemdev
rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
r? libs-api because this is related to a future API change, I'm sure if this is the best way to proceed.
We discussed this in the @rust-lang/libs-api meeting and would be interested in having these traits in the standard library. An idea also came up to also provide blanket implementations of these traits for tuples of types implementing Borrow so that (String, String) could be indexed by (&str, &str).
We are however not willing to accept this PR as it is: we want these traits to be public and tracked by a tracking issue so that people can start experimenting with them on nightly.
cc @cuviper
Ok. I can create a tracking issue and update the PR to expose these traits in core with some blanket impls
This comment has been minimized.
This comment has been minimized.
8ee7f33 to
4dd7d91
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.
Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
4dd7d91 to
578b1b4
Compare
This comment has been minimized.
This comment has been minimized.
An idea also came up to also provide blanket implementations of these traits for tuples of types implementing
Borrowso that(String, String)could be indexed by(&str, &str).
Doesn't seem to be possible. (K1, K2) might be (&str, &str) which implements Borrow<(&str, &str)> which causes a conflict for Equivalent<(&Q1, &Q2)>
The job aarch64-gnu-llvm-19-1 failed! Check out the build log: (web) (plain enhanced) (plain)
Click to see the possible cause of the failure (guessed by this bot)
---- [rustdoc] tests/rustdoc/internal.rs stdout ----
------python3 stdout------------------------------
------python3 stderr------------------------------
13: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has internal/struct.S.html '//*[@class="stab unstable"]' ''
Encountered 1 errors
------------------------------------------
error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/rustdoc/internal" "/checkout/tests/rustdoc/internal.rs"
stdout: none
--- stderr -------------------------------
13: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has internal/struct.S.html '//*[@class="stab unstable"]' ''
Encountered 1 errors
------------------------------------------
---- [rustdoc] tests/rustdoc/internal.rs stdout end ----
An idea also came up to also provide blanket implementations of these traits for tuples of types implementing
Borrowso that(String, String)could be indexed by(&str, &str).Doesn't seem to be possible.
(K1, K2)might be(&str, &str)which implementsBorrow<(&str, &str)>which causes a conflict forEquivalent<(&Q1, &Q2)>
Right, impl<T> Borrow<T> for T gets in the way.
It would probably make a good example though, e.g. some kind of struct Lookup(&str, &str) newtype.
Could we make this work by moving the reference to Q into the trait? Playground
Uh oh!
There was an error while loading. Please reload this page.
Tracking issue: #145986
This introduces the Equivalent and Comparable traits into core, under the
comparable_traitfeature flag. This exposesComparablefor btree key lookups, with a blanket impl forBorrowkeys to remain backwards compatible.The one annoying thing is that we still need
Q: Ordin order to provide the panic messages for bad usage in range queries. One could likely address this but it would move the branch inside the search loop which might impact performance. Another approach is to move the condition out, but that won't help if we want the same helpful panic condition in any potential exposed API. Note that this check is not needed for memory safety (as we already have to deal with invalid Ord impls) so maybe such a check is not really that necessary. The alternative is we accept thisQ: Ordbound as not a significant limitation.