Skip to content
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

Odd sequence of BTreeSet impls and iter doesn't require T: Ord #47138

Closed
ghost opened this issue Jan 2, 2018 · 5 comments
Closed

Odd sequence of BTreeSet impls and iter doesn't require T: Ord #47138

ghost opened this issue Jan 2, 2018 · 5 comments

Comments

@ghost
Copy link

ghost commented Jan 2, 2018

From the docs for BTreeSet:

We see four impls, but there really should be just one. It's also interesting that iter doesn't require T: Ord. These impls are much different from the ones in BTreeMap, HashSet, and HashMap. 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 need T: Ord, but you can't construct a BTreeSet without the bound anyway. You can reach for unsafe to create a BTreeSet<T> even if T doesn't impl Ord, but it's very unlikely that such code exists in the wild. In any case, I'd consider the missing bound on iter simply a bug.

@vitalyd
Copy link

vitalyd commented Jan 2, 2018

It’s a breaking change because existing generic code that takes a BTreeSet<T> and calls iter() may not be specifying the T: Ord bound. With the proposed change, they would have to (even if they don’t otherwise care about the ordering).

@ghost
Copy link
Author

ghost commented Jan 2, 2018

You're right -- that didn't occur to me.

In that case, perhaps we should just group all the methods that do require T: Ord in a single impl block to make the generated docs cleaner.

@bluss
Copy link
Member

bluss commented Jan 2, 2018

Yes please group the methods into fewer impls. Whatever presents it better in docs, if the API is equivalent either way.

@vitalyd
Copy link

vitalyd commented Jan 2, 2018

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.

@scottmcm
Copy link
Member

scottmcm commented Jan 3, 2018

It's also interesting that iter doesn't require T: Ord.

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 <T> on your function.

kennytm added a commit to kennytm/rust that referenced this issue Jan 5, 2018
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants