Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#266 DateTime::checkLimit looks wrong

closed
nobody
5
2012-09-14
2009-06-10
Anonymous
No

Hi, browsing the source I saw DateTime::checkLimit() and it didn't look right. E.g., if _hour was 24, wouldn't you want checkLimit(_hour, _day, 23); to make _hour = 0 and _day = _day + 1? I think it would make _hour = 1.

(I've met Poco for the first time today. Looks interesting. I hope I'm not wasting your time with this.)

Discussion

  • Alex Fabijanic
    Alex Fabijanic
    2009-06-11

    _hour being 24 means there is an extra hour, so we want it to roll over. This code only rectifies the rarely encountered overflow of the algorithm in computeGregorian.

     
  • Alex Fabijanic
    Alex Fabijanic
    2009-06-11

    On a second thought, you are right - for zero-based units, we should roll over to zero if there is only a single unit overflow. Patch is attached and trunk code updated.

     
  • Alex Fabijanic
    Alex Fabijanic
    2009-06-11

     
    Attachments
  • I think the patch doesn't address the issue. Both the 'lower' and 'higher' results could potentially be incorrect. For example, if _hour was 23, the patch code will return with _hour = -1 and _day = _day + 1. The problem is that in all calls to checkLimit the limit parameter is one less than it needs to be: instead of checkLimit(_hour, _day, 23) it should be checkLimit(_hour, _day, 24), then the unpatched checkLimit would return the correct values. (Also all other calls should be changed, e.g. checkLimit(_microsecond, _millisecond, 1000), or add 1 to limit inside checkLimit.)

    If I am right then the test suite is also either incorrect or incomplete. If I am wrong, my apologies.

     
  • Alex Fabijanic
    Alex Fabijanic
    2009-06-13

    if _hour was 23, the patch code will return with _hour = -1 and _day = _day + 1.

    if _hour is 23 then

    lower == 23 and limit == 23

    so

    (lower > limit) == false

    and nothing happens in checkLimit().

     
  • You are right - I am mistaken.

    I guess my comments would still apply if _hour were 2 * 23, in which case _day = _day + 2 and _hour = -1. But I don't understand the code well enough to know whether _hour could ever be this big (or any of the other calls to checkLimit with lower = 2 * limit).

    Thanks for your quick responses!

     
  • Alex Fabijanic
    Alex Fabijanic
    2009-06-14

    whether _hour could ever be this big

    It could not. The normalization is protection against rare cases when the algorithm overflows by one due to floating point rounding. I have only seen it happen for microseconds, but once it happens, to remain within the invariant you have to cascade all the way up to year. Additionally, there are debug asserts after the call to normalize().

     
  • I have looked at this, and I found checkLimit() a bit strange, too.
    Shouldn't it be:

    void DateTime::checkLimit(short& lower, short& higher, short limit)
    {
    if (lower >= limit)
    {
    higher += short(lower / limit);
    lower = short(lower % limit);
    }
    }

    void DateTime::normalize()
    {
    checkLimit(_microsecond, _millisecond, 1000);
    checkLimit(_millisecond, _second, 1000);
    checkLimit(_second, _minute, 60);
    checkLimit(_minute, _hour, 60);
    checkLimit(_hour, _day, 24);

    if (_day > daysOfMonth(_year, _month))
    {
        _day -= daysOfMonth(_year, _month);
        if (++_month > 12)
        {
            ++_year;
            _month -= 12;
        }
    }
    

    }

     
  • and you could then also eliminate the if

    void DateTime::checkLimit(short& lower, short& higher, short limit)
    {
    higher += short(lower / limit);
    lower = short(lower % limit);
    }

    and it would be correct even if lower >= 2 * limit (up to the point the shorts overflow, of course).

     
  • Alex Fabijanic
    Alex Fabijanic
    2009-06-15

    Strictly speaking, yes, that would be the universally correct version covering the cases when overflow is double the limit value. Since remainder in computeGregorian() is < 1.0, the only case we can have is overflow by one due to rounding error.

    I have changed code as proposed by Guenter, inlined the checkLimits() and left the (lower >= limit) check for performance reasons - in vast majority of cases, normalization is not needed at all. Code is in the SVN trunk, rev. 1188

     
  • fixed in 1.3.6 as well.

     
  • Thanks guys.