-
-
Notifications
You must be signed in to change notification settings - Fork 753
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
Conversation
…ully this will compile on all platforms.
…d of down. Also need to limit the scope of the calendar to avoid overflows and maybe other nastiness.
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 :( |
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. |
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? |
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. |
…Pico now uses more or less memory.
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). |
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 |
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. |
Ok, great - thanks for this! |
There were two things wrong with the Date library
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.