-
Notifications
You must be signed in to change notification settings - Fork 7
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
Merge TaggedColor and CssColor #25
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,45 +3,49 @@ | |
|
||
//! A simple bitset. | ||
|
||
/// A simple bitset, for representing missing components. | ||
/// A simple bitset for representing missing components. | ||
#[derive(Default, Clone, Copy, PartialEq, Eq, Debug)] | ||
pub struct Bitset(u8); | ||
pub struct Missing(u8); | ||
Comment on lines
-6
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be clearer if this represented present (non-missing) components? 0 to represent "present" and 1 to represent "missing" seems somehow inverted. Presumably this is equivalent on a technical level? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps "missing" semantics make sense because the common case is "no missing components"? If so then perhaps the code could stay the same, but the method could be documented in terms of "which components are present (non-missing)" rather than (or in addition to) "which components are in the set". e.g. for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would also appreciate an/some example(s) of when components can be missing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add some docs about missing components. Inverting the boolean sense is equivalent at a technical level. I think the strongest case for this way ("missing") is that the default value of 0 makes sense. Incidentally, I considered and rejected some other representations for this. In particular, color.js uses |
||
|
||
impl Bitset { | ||
impl Missing { | ||
/// Returns `true` if the set contains the component index. | ||
pub fn contains(self, ix: usize) -> bool { | ||
(self.0 & (1 << ix)) != 0 | ||
} | ||
|
||
pub fn set(&mut self, ix: usize) { | ||
/// Adds a component index to the set. | ||
pub fn insert(&mut self, ix: usize) { | ||
self.0 |= 1 << ix; | ||
} | ||
|
||
pub fn single(ix: usize) -> Self { | ||
/// The set containing a single component index. | ||
pub fn singleton(ix: usize) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've gone back to |
||
Self(1 << ix) | ||
} | ||
|
||
pub fn any(self) -> bool { | ||
self.0 != 0 | ||
/// Returns `true` if the set contains no indices. | ||
pub fn is_empty(self) -> bool { | ||
self.0 == 0 | ||
} | ||
} | ||
|
||
impl core::ops::BitAnd for Bitset { | ||
impl core::ops::BitAnd for Missing { | ||
type Output = Self; | ||
|
||
fn bitand(self, rhs: Self) -> Self { | ||
Self(self.0 & rhs.0) | ||
} | ||
} | ||
|
||
impl core::ops::BitOr for Bitset { | ||
impl core::ops::BitOr for Missing { | ||
type Output = Self; | ||
|
||
fn bitor(self, rhs: Self) -> Self { | ||
Self(self.0 | rhs.0) | ||
} | ||
} | ||
|
||
impl core::ops::Not for Bitset { | ||
impl core::ops::Not for Missing { | ||
type Output = Self; | ||
|
||
fn not(self) -> Self::Output { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad merge, right?