This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] [BZ #16141] strptime %z: fix rounding, extend range to +/-9959
- From: James Perkins <james at loowit dot net>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: GNU libc <libc-alpha at sourceware dot org>, Mike Frysinger <vapier at gentoo dot org>, Paul Eggert <eggert at cs dot ucla dot edu>, Will Newton <will dot newton at linaro dot org>
- Date: Thu, 13 Aug 2015 17:47:41 -0700
- Subject: Re: [PATCH v4] [BZ #16141] strptime %z: fix rounding, extend range to +/-9959
- Authentication-results: sourceware.org; auth=none
- References: <1439339237-22204-1-git-send-email-james at loowit dot net> <55CCC9A2 dot 6000501 at redhat dot com>
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