On Wed, Jun 22, 2016 at 06:07:00PM -0500, Chris Muller wrote:
Hi David, thanks for proceeding with caution, clearly, this is a core framework upon which everyone's Squeak everything depends. Some questions:
DateAndTimeLeapTest>>testAsSeconds - Discussion required. I claim that #asSeconds can only make sense if it means elapsed seconds since the epoch. The prior implemention and the test are both wrong.
Is there a context where the number of seconds since the epoch would ever "make sense" except to the software which needs to compare it to another result of #asSeconds? Why does it matter what number we start at?
DateAndTimeLeapTest>>testHash - DateAndTime>>hash works differently now. Test should be either updated or eliminated.
DateAndTimeTest>>testPrecision - Discussion required. It is simply wrong to assume that two calls to DateAndTime class>>now will always answer different values, and it is even more wrong to fudge the clock accessors to make it appear to be doing this when it is not. Yes I know that certain database applications require distinct time stamps, but hacking the actual system clock is not the right thing to do.
TimespanDoTest>>testDatesDo - Need to change the test setUp method. The original intent is to test date iteration using a time span begining with a UTC midnight. Test will pass if the start time is constructed accordingly. Note however that date iteration is suspect for durations starting at times other than midnight in the local time zone, so additional tests may be needed.
TimespanDoTest>>testNext - Same issue as testDatesDo, fix the setUp method and the test will pass, but additional tests may be needed.
Can you clarify why the setUp method needs to be changed? What is the core difference between your framework and Chronology which makes this test not work as-is?
I believe that the following would be consistent with the intent of the test, and will permit the tests to pass. The test is trying to interate dates through a time span of 91 days, and was written such that the time span would start on a midnight, such that it would iterate over 91 (not 92) dates. The change below accomplishes that in the #setUp.
TimespanToTest>>setUp aDate := DateAndTime year: 2003 month: 01 day: 07 hour: 0 minute: 0 second: 0 offset: Duration zero. aDuration := Duration days: 91 hours: 0 minutes: 0 seconds: 0 nanoSeconds: 0. aTimespan := Timespan starting: aDate makeUTC duration: aDuration
I want to understand whether there are any compatibility issues to worry about other than the internal format and the #hash calculation?
None that I can think of, but I have been looking at this much too long and I might easily be missing something. Let me know if you can think of anything, more eyeballs on this would be helpful.
The necessary #primitiveUtcWithOffset is present in all Cog/Spur/interpreter VMs. It is an optional named primitive (not numbered). If not present, the fallback code uses the Posix epoch for all new time values, resulting in a runnable image but with incorrect time stamps for new DateAndTime instances. Falling back to the original millisecond clock logic and primitives is possible, but it seemed to me to be an unnecessary complication so I got rid of it.
Wrong timestamps are a big deal. It would be better to have an unrunnable image than incorrect timestamps. The former makes a visible issue, the latter, an invisible, insidious one..
I think the image must remain runnable, even if in a degraded mode. Otherwise you cannot fix it. I think (but am not sure) that someone would quickly notice if all new DateAndTime instances are 45 years old.
We could restore the backward compatibility for running in fallback mode on the old millisecond primitives. I initially planned to keep that, but ended up concluding that it was just to much ugly old cruft to keep in the image. On the other hand, you may have noticed that I tend to be a big fan of backward compatibility, so you probably would not have a hard time convincing me if you think it is a good thing to do ;-)
Does anyone think that adding UTCDateAndTime to trunk is /not/ a good thing to do?
Until we're sure its a good idea, it's /not/ a good thing to do. I would like to encourage us all to take the due time to really understand all the differences, and would ask someone to produce documentation (on the wiki) which explains exactly what developers need to do to, 1) update their application code, 2) update any persistent instances from Chronology of DateAndTime, etc.
Agreed.
And you have just confirmed my hypothesis that asking for positive feedback never works, while tossing out some flame bait can be tremendously effective. Maybe I'll try running for president next ;-)
I think this will be a lot of work for me, but the reward will be faster dates and times, which is a good thing, thanks for pushing on this Dave.
I think that it actually will provide a good improvement. One thing I really do not know is how much of the improvement will be perceptible to the user in everyday activities. I am expecting speedups for things related to source code lookups, but I don't know if the low level speed improvements that I measure will translate into meaningful differences in real life.
Do we have any benchmarks that might show if the DateAndTime speedups will result in improvements that would be noticed by an end user?
Dave