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

["Carlos O'Donell" <carlos at redhat 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 <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.

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
before commit.

>> 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
>>>       +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.

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.

Cheers,
Carlos.