-
Notifications
You must be signed in to change notification settings - Fork 174
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
Use packed data within ChineseBasedYearInfo #4465
Conversation
5f5eff6
to
cd80a7d
Compare
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.
Nice
/// ``` | ||
/// | ||
/// Where the New Year Offset is the offset from ISO Jan 21 of that year for Chinese New Year, | ||
/// the month lengths are stored as 1 = 30, 0 = 29 for each month including the leap month. | ||
#[derive(Debug, Copy, Clone)] | ||
/// | ||
/// Should not |
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.
Should not what?
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.
... I have absolutely no idea
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.
I'll fix this in the upcoming PR; this type will probably move to provider/chinese_based.rs anyway
!month_lengths[12] || leap_month_idx.is_some(), | ||
"Last month length should not be set for non-leap years" | ||
); | ||
debug_assert!(ny_offset < 32, "Year offset too big to store"); |
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.
Thought: You don't have much wiggle room here; I think you are making use of all 32 values :)
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.
Potentially 31, but yes. Andrew and I spent some time researching this but I do plan to run a test on every year to check this property.
prev_month_lengths += long_month_bits.count_ones().try_into().unwrap_or(0); | ||
prev_month_lengths |
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.
Praise: clever use of count_ones()
Part of #3933
This does not yet add data loading, but it gets very close to it.
This PR makes the size of Date not bloat too much, without requiring much computation for any of the getters -- they're all basic bit operations.