Reviewing merge request #2282: Fix QDate::isLeapYear() for years < 1
The current formula for isLeapYear() assumes that years -4, -8, etc are
leap years in the Julian Calendar. This is not the case, leap years in
fact fall in -1, -5, -9, etc as there is no year 0 in the Julian
Calendar.
- Requested by:
John Layt - Status: Merged
- Source repository: odysseus-clone:qdatetime-fixes
- Target repository: qt:master
- Created at: January 7th, 2010 23:22
Commits that would be merged:
Comments
We are wondering the use case of that patch (correct, but that still change behaviour.)
(We have guessed you might be building a GUI for a time machine, is that right?)
Also, the test take almost one second to run. maybe you can make it a bit faster (by covering a smaller range, or by doing bigger step)
The fix is technically correct. If year 4 is leap, then the year before year 1 is leap too (1 B.C.).
But Olivier’s suggestion is also true. Please have the roundtrip test do only a few samples, not bruteforce every single date.
Ah, time travel is a necessary first step for world domination :–)
While it is a change in behaviour for isLeapYear(), it just makes it consistent with the Julian formulas used in julianDayFromDate(), and getDateFromJulianDay().
Test modified to cover only 405 or so important years instead of 14,000.
Hmmm, I guess I need to update the status here? Assume I need to set the status back to reviewing? Or New? Or do I create a whole new request?
Well, my guess is that you need to set the state of your merge request to “New” so that some Qt developers see it again.
Please do a bigger step than trying 5373484 days.
Other than that, the patch looks fine.
Revision 6 / commit 9878136 is only testing approx 405 important years * 365 days = 147,825 days, not 14,000 years / 5 million days as it was in revision 5.
Merged in 4.7 (568475d98d77c3ade027eb88e571ca8b84088002)
Add a new comment:
Login or create an account to post a comment