Re: [PATCH v4] [BZ #16141] strptime %z: fix rounding, extend range to +/-9959

[James Perkins <james at loowit dot net>]

Thanks for the extensive and thorough review, Carlos.

On Thu, Aug 13, 2015 at 9:45 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/11/2015 08:27 PM, James Perkins wrote:

> After we fix the testing we can do a thorough polish of any final details.

If you feel that base patch needs any final tweaks, I would very much
like to hear the details.

> As mentioned above, this needs to be more thorough.

I feel significant test enhancements are separate from the base patch
which fixes the problem in strptime and adds a few tests for it.

>> 3) Add zone offset values to time/tst-strptime2.c.
>>
>>   * Test minutes evenly divisible by three (+1157) and not evenly
>>     divisible by three (+1158 and +1159).
>>   * Test offsets near the old and new range limits (-1201, -1330, -2459,
>>     -2500, -99, -9959, +1201, +1330, +1400, +1401, +2559, +2600, +99,
>>     and +9959)
>
> Not enough IMO. We might break this again in the future. I'd like to see
> a thorough iteration of the values.

>>       * time/strptime2.c (tests): Modify and add tests for the
>>       strptime %z input field descriptor, specifically conversion of
>>       minutes to seconds and validating an offset range of -9959 to
>>       +9959.

I see a small omission in my strptime_v4 patch here where it's
actually time/tst-strptime2.c. So, that's one thing I need to fix.

> I created a small test case to isolate this code and validated it for
> all values that were valid and your change matches the expected values
> computed using double precision.

In exploring test enhancements, I didn't need to use floating point.

> This test needs to be extended to cover all somehow. I don't want to
> come back to this in 5 years when it breaks again.
>
> Is that asking too much?

I'll send a patch right along for an extended/iterative test case.

The test rework I did covers the entire range of digit values from
-9999 to +9999, also -999 to +999, -99 to +99, -9 to +9, and the lack
of any digit found. In short, it goes a bit beyond what you asked, but
I feel better testing an entire swath of input that users, valid and
invalid, that users may be expected to provide strptime.

Cheers,
James