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

Sets should not impl Ord, and should impl PartialOrd differently (?) #16570

Closed
Gankro opened this Issue Aug 18, 2014 · 7 comments

Comments

Projects
None yet
3 participants
@Gankro
Copy link
Contributor

Gankro commented Aug 18, 2014

Currently, sets that have some notion of an ordering over their elements (TreeSet and BitvSet, at least) implement Ord in terms of the lexicographic ordering of their contents. However, as @apoelstra notes in #16559 it may be more natural for sets to be ordered by inclusion. That is, a<=b if a subseteq b.

However, there is no total ordering over set inclusion. It instead forms a diamond-shaped DAG. Thus, Sets would not implement Ord under this scheme. In fact, almost all pairs of sets would have cmp yield None, which makes it a not-very-useful operator for generic comparison. Further, inclusion relationships are already provided by the actual Set api.

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Aug 18, 2014

CC @aturon

@apoelstra

This comment has been minimized.

Copy link
Contributor

apoelstra commented Aug 18, 2014

Discuss thread with these points listed:
http://discuss.rust-lang.org/t/stdlib-has-many-ord-implementations-on-things-that-arent-totally-ordered/394

Perhaps as part of the collections restructuring support for multiple orderings will have to be invented somehow.

@thestinger

This comment has been minimized.

Copy link
Contributor

thestinger commented Aug 18, 2014

I think all containers should implement ordering as a lexicographic comparison. There are separate methods for checking for a subset / superset already, and we would lose the nice O(n) worst-case lexicographic comparisons on TreeSet. I don't understand why lexicographic comparisons are "crazy" semantics, as it's what I expect for all ordered containers. It would be very surprising if Rust's implementation was different than the C++ standard library, and would lead to bugs based on an assumption of a familiar implementation.

@thestinger thestinger added the A-libs label Aug 18, 2014

@apoelstra

This comment has been minimized.

Copy link
Contributor

apoelstra commented Aug 18, 2014

@thestinger Lexicographic comparisons are crazy for set types because sets are unordered and the only natural ordering used on them is subset inclusion.

For ordered containers, you're right, they make sense (and set inclusion doesn't).

@thestinger

This comment has been minimized.

Copy link
Contributor

thestinger commented Aug 18, 2014

The sole reason TreeSet exists is to provide an ordered set. It is an ordered container and a lexicographic comparison is both sensible and very useful. It would be surprising to do anything else, because that's what ordered sets in the C++ standard library do.

@apoelstra

This comment has been minimized.

Copy link
Contributor

apoelstra commented Aug 18, 2014

Ok, thanks. I was misunderstanding the implied semantics of -Set types, and also wasn't aware of their STL equivalents.

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Aug 18, 2014

Alright, satisfied that this has been addressed: Lexicographic ordering is useful and standard.

@Gankro Gankro closed this Aug 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.