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

Bugfix for #2456 (PR #2230) - Date library bug fixes #2459

Merged
merged 13 commits into from
Jan 29, 2024

Conversation

deirdreobyrne
Copy link
Contributor

There were two things wrong with the Date library

  1. The algorithm required divisions to round down, but integer division rounds-towards-zero. This was creating artifacts like negative month numbers.
  2. There was no range checking on the inputs. Inputs large enough in magnitude could cause overflows in some of the integer operations.

Also the spelling on the function "fromCalendarDate" has been corrected. And the documentation for the "setMonth" method was wrong.

Date now implements the Gregorian calendar for over 1.2 million years either side of the present. Inputs outside that range will throw an error. Note that the Gregorian calendar actually came into existence in 1582, but memory is too tight on some platforms (notably PICO_R1_3) to do anything intelligent with that fact.

@deirdreobyrne
Copy link
Contributor Author

All I did was add in the test case I was sent by email - no changes were made to the base code - and PICO_R1_3 is failing to build due to lack of memory.

Converting to draft, and seeing if I can save a few bytes :(

@deirdreobyrne deirdreobyrne marked this pull request as draft January 29, 2024 12:04
@deirdreobyrne
Copy link
Contributor Author

I've further restricted the time period for which Date() works on PICO_R1_3. This enabled getting rid of code which does round-down integer divide on that platform.

Note I have no way of running the code to test the tests/test_date.js script for the PICO_R1_3 build.

@deirdreobyrne deirdreobyrne marked this pull request as ready for review January 29, 2024 13:16
@gfwilliams
Copy link
Member

Thanks! I'm amazed those changes saved enough space!

Just thinking - if you made INTEGER_DIVIDE_FLOOR into a function, could that save enough space that you didn't have to ifdef it for the Pico?

@deirdreobyrne
Copy link
Contributor Author

Doubtful - as INTEGER_DIVIDE_FLOOR is simply (a/b) on the Pico (in other words, it actually divides-towards-zero, which is why the Date() library has to be limited to after 1601 on that platform), and the STM32F401CDU6 has a divide instruction, so all the moving and shaking that needs to happen for a function call would probably take up more space.

However since I've realised one of the tests will definitely fail on the Pico (the one you sent me - new Date(-12,8)) I'll make some changes later and see. Although the cross-compiler I use for the Pico generates code far bigger than whatever github uses, so I'll have to use github to see what, if any, difference it makes.

@deirdreobyrne deirdreobyrne marked this pull request as draft January 29, 2024 16:48
@deirdreobyrne
Copy link
Contributor Author

I believe the numbers show I was right - 327616 vs 327640 bytes for the Pico.

However splitting the INTEGER_DIVIDE_FLOOR into a function does save bytes on all other platforms (as the integerDivideFloor function is reasonably complicated, so there are fewer bytes to call the function than to perform it).

@deirdreobyrne deirdreobyrne marked this pull request as ready for review January 29, 2024 16:56
@gfwilliams
Copy link
Member

However splitting the INTEGER_DIVIDE_FLOOR into a function does save bytes on all other platforms

Yes - what I meant really was maybe with INTEGER_DIVIDE_FLOOR as a function, enough bytes were saved you wouldn't have to do anything special for the Pico at all and could just use the normal code?

No problem about pico testing - but if you think you really do still need ESPR_LIMIT_DATE_RANGE even with the INTEGER_DIVIDE_FLOOR function then you could I guess at least build a platform that you're confident with, with the ESPR_LIMIT_DATE_RANGE define in it

@deirdreobyrne
Copy link
Contributor Author

Yes - what I meant really was maybe with INTEGER_DIVIDE_FLOOR as a function, enough bytes were saved you wouldn't have to do anything special for the Pico at all and could just use the normal code?

Doing so consumed 96 more bytes on my compile of the pico code, but there are only 34 bytes left!

I've done the pico testing as you suggested - all is good.

And note that I extended the Date() range for the Pico back to 1500 - back to before the introduction of the Gregorian calendar.

I think it's about as good as we can make it.

@gfwilliams
Copy link
Member

Ok, great - thanks for this!

@gfwilliams gfwilliams merged commit fcd68f6 into espruino:master Jan 29, 2024
15 checks passed
gfwilliams added a commit that referenced this pull request Jan 29, 2024
@deirdreobyrne deirdreobyrne deleted the date_limit branch January 29, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants