Fix bug in POSIX time zone calculations #658
Conversation
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
I'm not quite sure how this overflow works, can you add comments?
|
|
||
| impl PosixDate { | ||
| pub(crate) fn from_rule(rule: &Rule) -> Self { | ||
| pub(crate) fn from_rule(rule: &Rule) -> (Self, i64) { |
There was a problem hiding this comment.
nit: document return type
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
Still don't fully understand why something would be "off the target weekday" but that's fine
zoneinfo/src/posix.rs
Outdated
| QualifiedTime::Local(time) => time, | ||
| QualifiedTime::Standard(standard_time) => standard_time.add(rule.save), | ||
| QualifiedTime::Universal(universal_time) => universal_time.add(offset).add(savings), | ||
| QualifiedTime::Local(time) => time.add(Time::from_seconds(time_overflow)), |
There was a problem hiding this comment.
nit: probably just add to the time outside the match, let time = time.add(overflow)
| // This ensures that day_of_month being 1 aligns with Sun = 0, for | ||
| // Sun>=1 purposes. | ||
| // | ||
| // The primary purpose for this approach as noted in zic.c is to support |
There was a problem hiding this comment.
link? I don't find any such comment but I might not be looking hard enough
There was a problem hiding this comment.
The comment is from the commit history. I can link to it.
| impl WeekDay { | ||
| pub(crate) fn from_u8(value: u8) -> Self { | ||
| match value { | ||
| 0 => Self::Sun, | ||
| 1 => Self::Mon, | ||
| 2 => Self::Tues, | ||
| 3 => Self::Wed, | ||
| 4 => Self::Thurs, | ||
| 5 => Self::Fri, | ||
| 6 => Self::Sat, | ||
| _ => unreachable!("invalid week day value"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like, even for pub(crate) this should return a Option<Self> where the caller should be responsible for doing the unreachable assertion, which holds in the case of zoneinfo/src/posix.rs:180
There was a problem hiding this comment.
Probably this is something that needs fixing in more than one area. Maybe I can look into that
There was a problem hiding this comment.
It's probably fair. I debated it myself, but decided against it because it was an internal operation that is ultimately unreachable.
But that being asserted by the caller is definitely a better argument.
This is going to be a first in a string of PRs to implement a zero-copy compiled data zoneinfo provider.
Fixes a bug in the POSIX time zone calculations where we weren't correctly handling the time overflow on some edge cases.