Remove PartialOrd and Ord implementations from WindowId, DeviceId, MonitorHandle and VideoModeHandle#3866
Remove PartialOrd and Ord implementations from WindowId, DeviceId, MonitorHandle and VideoModeHandle#3866daxpedda wants to merge 2 commits into
PartialOrd and Ord implementations from WindowId, DeviceId, MonitorHandle and VideoModeHandle#3866Conversation
Removes `PartialOrd` and `Ord` from: - `WindowId` - `DeviceId` - `FingerId` - `MonitorHandle` - `VideoModeHandle`
daxpedda
requested review from
MarijnS95,
jackpot51,
kchibisov,
madsmtm and
notgull
as code owners
|
I ran into this in the NDK crate as well, PR rust-mobile/ndk#483 (reviews welcome!). We have some pointer wrappers (and a few random non-sortable enums) that derive |
madsmtm
left a comment
madsmtm
left a comment
There was a problem hiding this comment.
I believe that there is value in PartialOrd and Ord for 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 in BTreeMap/BTreeSet.
| self.monitor().cmp(&other.monitor()).then( | ||
| self.size() | ||
| .cmp(&other.size()) | ||
| .then( | ||
| self.refresh_rate_millihertz() | ||
| .cmp(&other.refresh_rate_millihertz()) | ||
| .then(self.bit_depth().cmp(&other.bit_depth())), | ||
| ) | ||
| .reverse(), |
There was a problem hiding this comment.
I agree that VideoModeHandle's Ord is weird (and likely incorrect, I could imagine two video modes that have the same monitor, size, refresh rate and bit depth, but differed on some other parameter, in which case Ord would say they were equal, but Eq would say they weren't).
The argument is that without semantic meaning its not a good idea to implement But yes, the implementation is stable and correct, but I don't know what the point is of being able to store it in 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 Personally, my view until now was to only implement |
madsmtm
left a comment
madsmtm
left a comment
There was a problem hiding this comment.
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 implement PartialOrd and Ord, including BTreeMap itself.
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.
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.
| // EnumDisplaySettingsExW can return duplicate values (or some of the | ||
| // fields are probably changing, but we aren't looking at those fields | ||
| // anyway), so we're using a BTreeSet deduplicate | ||
| let mut modes = BTreeSet::<RootVideoModeHandle>::new(); | ||
| let mut modes = Vec::new(); |
There was a problem hiding this comment.
I think we may still need to deduplicate here?
There was a problem hiding this comment.
Done, though I believe that we should actually not de-duplicate here but instead expose the underlying type with an extension specific trait; a matter for another time.
Co-Authored-By: Mads Marquart <mads@marquart.dk>
Wish
The only thing I could think of is different implementation speeds, but |
|
|
Right, and that piggybacks off of the lexicographical ordering of the underlying slices. Not of the pointer that could change. |
4 participants
As discussed in the last meeting, this PR removes
PartialOrd/Ordfrom:WindowIdDeviceIdFingerIdMonitorHandleVideoModeHandleThe reason being that
Ordis supposed to tell users something meaningful, which it doesn't in this case. E.g. what does comparing the order of twoDeviceIds mean?On
VideoModeHandlethere was actually some useful implementation, but it had a couple of issues:VideoModeHandleis a handle and doesn't contain the data readily available (on most backends). We might want to consider implementingOrdagain onVideoModeData.FingerIdwould have been useful to compare withOrdif 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.