Derive common traits for number, range and enum types #315
Conversation
src/structures/idt.rs
Outdated
| @@ -1001,7 +1001,7 @@ impl fmt::Debug for SelectorErrorCode { | |||
| /// The possible descriptor table values. | |||
| /// | |||
| /// Used by the [`SelectorErrorCode`] to indicate which table caused the error. | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
I don't think there is a logical ordering for these variants.
src/structures/paging/frame.rs
Outdated
| @@ -133,7 +133,7 @@ impl<S: PageSize> Sub<PhysFrame<S>> for PhysFrame<S> { | |||
| } | |||
|
|
|||
| /// An range of physical memory frames, exclusive the upper bound. | |||
| #[derive(Clone, Copy, PartialEq, Eq)] | |||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
Unfortunately there is no reasonable ordering for range types. For example, should 11..15 be considered smaller or larger than 12..13? Deriving PartialOrd here would order by the start bound first, which would make 1..999999 smaller than 2..3, which can be surprising.
See also the range type in Rust's standard library, which does not implement PartialOrd either.
Deriving Hash for range types seems fine.
There was a problem hiding this comment.
My initial reasoning with this was there often isn't an obvious ordering that makes sense, but still providing one anyway could still be useful eg to store them as keys in a BTreeMap where the meaning of the ordering doesn't need to have any meaning but there has to be an ordering.
In hindsight Range not implementing is a reasonable argument for not implementing PartialOrd/Ord that to avoid confusion.
There was a problem hiding this comment.
Yeah, for BTreeMap it doesn't really matter, but PartialOrd/Ord impls are also used in other places, e.g. in sort_unstable or Iterator::max, where the ordering does matter. So I think it's better to implement the traits only if there really is some logical ordering between the elements. For storing things in a BTreeMap users can still add a custom OrdWrapper type with an arbitrary Ord implementation.
src/structures/paging/frame.rs
Outdated
| @@ -175,7 +175,7 @@ impl<S: PageSize> fmt::Debug for PhysFrameRange<S> { | |||
| } | |||
|
|
|||
| /// An range of physical memory frames, inclusive the upper bound. | |||
| #[derive(Clone, Copy, PartialEq, Eq)] | |||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
Same point as above: I don't think that deriving PartialOrd/Ord makes sense here.
src/structures/paging/page.rs
Outdated
| @@ -279,7 +279,7 @@ impl<S: PageSize> Sub<Self> for Page<S> { | |||
| } | |||
|
|
|||
| /// A range of pages with exclusive upper bound. | |||
| #[derive(Clone, Copy, PartialEq, Eq)] | |||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
Same point as above: I don't think that deriving PartialOrd/Ord makes sense for range types.
src/structures/paging/page.rs
Outdated
| @@ -332,7 +332,7 @@ impl<S: PageSize> fmt::Debug for PageRange<S> { | |||
| } | |||
|
|
|||
| /// A range of pages with inclusive upper bound. | |||
| #[derive(Clone, Copy, PartialEq, Eq)] | |||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
Same point as above: I don't think that deriving PartialOrd/Ord makes sense for range types.
src/lib.rs
Outdated
| @@ -67,7 +67,7 @@ pub mod registers; | |||
| pub mod structures; | |||
|
|
|||
| /// Represents a protection ring level. | |||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | |||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
There was a problem hiding this comment.
I'm not sure about the PartialOrd/Ord derives: When talking about ring 0, ring 3, etc, it makes sense that Ring3 is considered larger than Ring0. However, the name of the type — PrivilegeLevel — kind of indicates the opposite ordering: One might reasonably expect that the highest privilege level has the most privileges, i.e. that Ring0 is the highest level.
Perhaps it's better to not derive PartialOrd here in order to prevent confusion? Users can still introduce their own wrapper types with an PartialOrd implementation if they want.
phil-opp
added
waiting-on-author
Rust's api guidelines recommend eagerly implementing common traits. There has already been some discussion #43 whether implementing all traits is always the best idea.
This pr add derives for
Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hashto some number and range types. These types are newtypes around integers and don't have any implementation details to hide, so adding these derives should be unobjectionable. This pr also adds the same derives to some enum types.Initially I only wanted to add theses derives to
Pcid, but I noticed that other types were also missing some of them and decided to add them as well.