This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH v4] [BZ #16141] strptime %z: fix rounding, extend range to +/-9959
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: James Perkins <james at loowit dot net>
- 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: Fri, 14 Aug 2015 10:30:29 -0400
- 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> <CAJ2jFj7ewLrER7eSQjVg5xMSEZ=6Do2dWEGc4L5wxYGCr6Ot3Q at mail dot gmail dot com>
On 08/13/2015 08:47 PM, James Perkins wrote:
> Thanks for the extensive and thorough review, Carlos.
> On Thu, Aug 13, 2015 at 9:45 AM, Carlos O'Donell <email@example.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.
My apologies, I did a thorough review, so there shouldn't be anything left,
but I usually say the above as "boiler plate" since something might come
up and I don't want to say "It's perfect" until we get to that last version
>> 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.
I see you extended the test case to cover -9999 to +9999 which exceeds
my expectations. I appreciate your professional response to this, and
I guarantee you the additional testing will reap benefits in the future.
Keep in mind that as the reviewer I see (a) an extension of the accepted
inputs and (b) only light coverage of those new inputs, where what I really
want is for this problem never to come back again.
I appreciate deeply your commitment to testing the entire range.
>>> 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
> 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.
That's OK. I was simply giving some background to the independent testing
that I did on my end. As someone who has also worked on compilers I like
to use two distinct types since it avoids common mode failures.
>> 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.
Exactly. Thank you. I'll review the test case, and then commit the
test case and the new fixes at the same time.