icu4x
icu4x copied to clipboard
Fix negative numbers across icu_calendar
Poking around, I keep finding places in icu_calendar where we do the wrong thing with negative numbers. We need to fix this, since one value proposition of ICU4X's calendar crate is that we generate correct results across the range required by Temporal.
I made a branch, iso-negative, with some test cases that are currently failing.
Actions on this ticket:
- [x] Clone my branch and change the code so that the tests I added can all pass
- [ ] Add tests to cover all operations that involve signed integers
- [ ] Fix code that these tests reveal to be broken, perhaps by using the new
div_rem_euclidhelper function (#2704)
Related: #2151
CC @pt2121 @Manishearth @pandusonu2
I ended up fixing half of this whilst attempting to fix the coptic issue
Pretty sure that check(-1828, -5, 12, 31); is incorrect; I just ran the original lisp code and it wants it to be 30.
@sffc where did you get these fixed values from?
Oh, though it might be a zero-indexed vs 1-indexed difference, argh
Nope, we're both 1-indexed.
I calculated the fixed values manually.
- Year 0: Fixed Days -1 through -366 (it is a leap year)
- Year -1: Fixed Days -367 through -731
- Year -2: Fixed Days -732 through -1096
- Year -3: Fixed Days -1097 through -1461
- Year -4: Fixed Days -1462 through -1827 (leap year)
- Year -5: Fixed Days -1828 through -2192
Ah, there's your problem, fixed day 0 is not Jan 1 1 AD (that's fixed 1), it is Dec 31 1 BC
I fixed it in the tests
https://github.com/unicode-org/icu4x/issues/2703 fixes most of this, except for week_of.
I didn't really add many tests though. I think we ought to fuzz pre-https://github.com/unicode-org/icu4x/issues/2703 code and include those testcases.
#3477 handles Julian
@atcupps should this issue be closed now, or are there still bits left to be handled?
Here are the calendars that have included negative date testing:
- [x] Buddhist
- [x] Chinese
- [ ] Coptic
- [x] Dangi
- [ ] Ethiopian
- [x] Gregorian
- [x] Indian
- [ ] Islamic
- [x] Iso
- [ ] Japanese
- [x] Julian
- [x] Persian
So there's still quite a few more to be done.
There are still bugs involving negative dates. See https://github.com/unicode-org/icu4x/pull/4894
A good next step would be to scrub the crate of any remaining % operations.