Fix ISO weekday calculations in negative years #4894
Conversation
|
Adding @echeran as reviewer since @Manishearth is out of office this week. |
There was a problem hiding this comment.
There are five more instances of % in the calendar component: https://github.com/search?q=repo%3Aunicode-org%2Ficu4x+%25+path%3Acomponents%2Fcalendar%2Fsrc%2F&type=code. Please clean them up as well, either by using rem_euclid, or by adding a comment showing that the left argument is non-negative.
| @@ -135,7 +135,9 @@ impl Calendar for Iso { | |||
|
|
|||
| // The days of the week are the same every 400 years | |||
| // so we normalize to the nearest multiple of 400 | |||
| let years_since_400 = date.0.year % 400; | |||
| let years_since_400 = date.0.year.rem_euclid(400); | |||
| debug_assert!(years_since_400 >= 0); // rem_euclid returns positive numbers | |||
There was a problem hiding this comment.
it does, let's not assert std guarantees.
There was a problem hiding this comment.
I suspected you might leave a comment like this :) ... I still have a slight preference for assertions on code invariants since as casts can be error-prone
|
The tracking issue is #2703. Let's merge this improvement. I might go ahead and work on the rest in a follow-up PR. |
|
The rest is in #4904 |
Fixes #4893
I thought we had already scrubbed the code of all non-euclidean calculations, but I guess not.
Thanks @anba for noticing this!