rust-windowing
Remove `PartialOrd` and `Ord` implementations from `WindowId`, `DeviceId`, `MonitorHandle` and `VideoModeHandle`
As discussed in the last meeting, this PR removes PartialOrd/Ord from:
-
WindowId -
DeviceId -
FingerId -
MonitorHandle -
VideoModeHandle
The reason being that Ord is supposed to tell users something meaningful, which it doesn't in this case. E.g. what does comparing the order of two DeviceIds mean?
On VideoModeHandle there was actually some useful implementation, but it had a couple of issues:
- The significance to a user was unclear because it wasn't documented in what order we are ordering them. The alternative to this PR is to just document this behavior.
- Significant overhead because
VideoModeHandleis a handle and doesn't contain the data readily available (on most backends). We might want to consider implementingOrdagain onVideoModeData. - Unclear benefits; users can sort them based on their specific needs themselves.
FingerId would have been useful to compare with Ord if the ID actually signifies the finger used in order, in which case we should expose that information directly to the user as well. It isn't clear yet if every backend functions this way, something to ponder about when we overhaul touch events.
I ran into this in the NDK crate as well, PR https://github.com/rust-mobile/ndk/pull/483 (reviews welcome!). We have some pointer wrappers (and a few random non-sortable enums) that derive Ord while others do not. After planning on removing them - because they likewise do not have any semantical meaning - this does break the ability to store these pointers inside structures like BTreeMap.
I believe that there is value in
PartialOrdandOrdfor all of these types! Even though the ordering may not have semantic meaning, it is still stable and correct (if we're comparing e.g. pointer addresses), and as said, allows using these types inBTreeMap/BTreeSet.
The argument is that without semantic meaning its not a good idea to implement Ord because it exposes API to the user with no apparent meaning.
But yes, the implementation is stable and correct, but I don't know what the point is of being able to store it in BTreeMap/BTreeSet if the order has no meaning.
I believe that whatever consensus we come here to, we should be able to formulate a rule that applies to our whole API, if we say that implementing PartialOrd/Ord without semantic meaning is desirable, then we should do so for all other types, if possible, as well.
Personally, my view until now was to only implement PartialOrd/Ord if it actually makes any sense, otherwise the API should be kept opaque unless otherwise desirable.
Thinking it over a bit more, I took a look at the APIs in
std, and it seems like a fair number of them also don't implementPartialOrdandOrd, includingBTreeMapitself.
Vec neither, likely because its pointer can (also) change when pushing new items causing a realloc().
I guess I was thinking of the C-COMMON-TRAITS API guideline, and wanted to follow that, but even that says to implement all the applicable traits.
Wish applicable was clarified a bit more explicitly here.
what the point is of being able to store it in
BTreeMap/BTreeSetif the order has no meaning.Hmm, yeah, I thought it would make the order consistent between e.g. re-runs of the same code, but that clearly can't happen if the ordering comes from a pointer.
The only thing I could think of is different implementation speeds, but HashMap is always faster than BTreeMap (except amortization): https://doc.rust-lang.org/std/collections/index.html#maps.
Thinking it over a bit more, I took a look at the APIs in
std, and it seems like a fair number of them also don't implementPartialOrdandOrd, includingBTreeMapitself.
Vecneither, likely because its pointer can (also) change when pushing new items causing arealloc().
Vec does implement PartialOrd.
Right, and that piggybacks off of the lexicographical ordering of the underlying slices. Not of the pointer that could change.