-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Odd sequence of BTreeSet impls and iter doesn't require T: Ord #47138
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
Comments
It’s a breaking change because existing generic code that takes a |
You're right -- that didn't occur to me. In that case, perhaps we should just group all the methods that do require |
Yes please group the methods into fewer impls. Whatever presents it better in docs, if the API is equivalent either way. |
Yeah that sounds reasonable. It’d be nice if rustdoc could do this grouping on the UI side so the code could be organized any which way. |
This is actually convenient for reducing typing if you need to loop through the set but don't care about changing it -- display it, sum it, whatever -- since you can just have |
…ichton Remove `T: Ord` bound from `BTreeSet::{is_empty, len}` This change makes the API for `BTreeSet` more consistent with `BTreeMap`, where `BTreeMap::{is_empty, len}` don't require `T: Ord` either. Also, it reduces the number of `impl`s for `BTreeSet`, making the generated documentation look much cleaner. Closes rust-lang#47138. cc @rust-lang/libs
From the docs for
BTreeSet
:We see four impls, but there really should be just one. It's also interesting that
iter
doesn't requireT: Ord
. These impls are much different from the ones inBTreeMap
,HashSet
, andHashMap
. I see no reason for why they should be written this way.Should we put all methods in just a single
impl<T> BTreeSet<T> where T: Ord
block instead?Technically, that'd be a breaking change since
iter
currently doesn't needT: Ord
, but you can't construct aBTreeSet
without the bound anyway. You can reach forunsafe
to create aBTreeSet<T>
even ifT
doesn't implOrd
, but it's very unlikely that such code exists in the wild. In any case, I'd consider the missing bound oniter
simply a bug.The text was updated successfully, but these errors were encountered: