espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.73k stars 741 forks source link

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

Closed deirdreobyrne closed 5 months ago

deirdreobyrne commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

gfwilliams commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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).

gfwilliams commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Ok, great - thanks for this!