Skip to content
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

Fix negative numbers across icu_calendar #2703

Closed
1 of 3 tasks
sffc opened this issue Oct 3, 2022 · 11 comments · Fixed by #4904
Closed
1 of 3 tasks

Fix negative numbers across icu_calendar #2703

sffc opened this issue Oct 3, 2022 · 11 comments · Fixed by #4904
Labels
C-calendar Component: Calendars good first issue Good for newcomers S-medium Size: Less than a week (larger bug fix or enhancement) T-bug Type: Bad behavior, security, privacy

Comments

@sffc
Copy link
Member

sffc commented Oct 3, 2022

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:

  • 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_euclid helper function (Add div_rem_euclid and use it in icu_calendar #2704)

Related: #2151

CC @pt2121 @Manishearth @pandusonu2

@sffc sffc added T-bug Type: Bad behavior, security, privacy good first issue Good for newcomers S-medium Size: Less than a week (larger bug fix or enhancement) C-calendar Component: Calendars labels Oct 3, 2022
@Manishearth
Copy link
Member

I ended up fixing half of this whilst attempting to fix the coptic issue

@Manishearth
Copy link
Member

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?

@Manishearth
Copy link
Member

Oh, though it might be a zero-indexed vs 1-indexed difference, argh

@Manishearth
Copy link
Member

Nope, we're both 1-indexed.

@sffc
Copy link
Member Author

sffc commented Oct 3, 2022

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

@Manishearth
Copy link
Member

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

@Manishearth
Copy link
Member

#2703 fixes most of this, except for week_of.

I didn't really add many tests though. I think we ought to fuzz pre-#2703 code and include those testcases.

@atcupps
Copy link
Contributor

atcupps commented Aug 1, 2023

#3477 handles Julian

@Manishearth
Copy link
Member

@atcupps should this issue be closed now, or are there still bits left to be handled?

@atcupps
Copy link
Contributor

atcupps commented Aug 1, 2023

Here are the calendars that have included negative date testing:

  • Buddhist
  • Chinese
  • Coptic
  • Dangi
  • Ethiopian
  • Gregorian
  • Indian
  • Islamic
  • Iso
  • Japanese
  • Julian
  • Persian

So there's still quite a few more to be done.

@sffc
Copy link
Member Author

sffc commented May 14, 2024

There are still bugs involving negative dates. See #4894

A good next step would be to scrub the crate of any remaining % operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-calendar Component: Calendars good first issue Good for newcomers S-medium Size: Less than a week (larger bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants