-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix incorrect comments for time range in CFCalendar.c #5274
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
base: main
Are you sure you want to change the base?
Conversation
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.
Looks like we already made this fix in swift-foundation's Date.swift
.
@swift-ci test |
@parkera Sorry, I had to push a correction to the PR after checking the definition of Julian day 0. It appears both swift-foundation's |
Out of curiosity, where does the date 506713-02-07 00:00:00 +0000 come from? It seems like the ICU universal time scale only has a range back to 29,000 BC and up to 29,000 AD, so the only thing I can think of is that 25,000 years or so were added to the back half of the range since dates before Julian day 0 weren't supported and that number (probably 53,286 AD or so) was rounded down to get 50,000 AD like the current documentation says, but then that maximum was miscomputed. On the other hand, things like the iOS Calendar app seem to work fine with the full range up to 506713 AD (as far as I can tell with the Gregorian calendar anyways) so it doesn't seem to be posing issues in practice if this theory is actually what happened. |
ICU has this implicit limit in their implementation that would cause some of their API to become no-op when the input exceeds the limit: https://github.com/swiftlang/swift-foundation-icu/blob/122f088ccda752062b4c44d4d8471ecb3638da32/icuSources/i18n/gregocal.cpp#L71
We decided to set a cap on our own instead of relying on their implicit limits so we know exactly what we're doing. The upper bound is just a random day in the far far future that sits safely below ICU's upper bound. |
@swift-ci please test |
The definitions of these macros do not match their documentation.
MIN_CALENDAR_TIME
does indeed represent Julian day 0 (proleptic Gregorian -4713-11-24 12:00:00 +0000 = November 24, 4714 BC), but it is documented incorrectly as -4713-01-01 12:00:00 +0000 which is 4714 BC instead of the correct year -4712 = 4713 BCMAX_CALENDAR_TIME
does not represent 50000-01-01 00:00:00 +0000 but in fact 506713-02-07 00:00:00 +0000This pull request proposes to keep the existing behavior but properly document it