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

[James Perkins <james at loowit dot net>]

On Fri, Aug 14, 2015 at 7:30 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/13/2015 08:47 PM, James Perkins wrote:
> > 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.

Completely understandable.... I now feel confident that you mentioned
every thing you could think of at the time you reviewed it. It's always fine
with me if something was overlooked the first time around.

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

I appreciate constructive feedback which may challenge my base
assumptions, so that I can make the most useful contribution (even if it is
a small contribution)... so this works for me. Thank you for the constructive
and professional review.

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

I agree that I want to fix this code and see it stay fixed. Your suggestion
to iteratively test the input range was a good one. It seems to me the code
to test the gamut of valid and invalid inputs isn't significantly more work
than just the valid input set.

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

This issue is fixed in my V5 patchset (coming very soon).

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

Avoiding common mode failures is a good point. I appreciate hearing your
background. I'm somewhat naive with my test writing skills as well as
floating point precision, rounding modes, and fuzzy equality tests, or I
might apply those techniques.

In my defense, I come from the embedded operating system architecture
support direction and that makes me tend to avoid running thousands of
floating point operations on processors that have none (anemic MIPS4kc
CPUs, for example). I've waited hours for test results many times.

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

Great! I see after doing them that my tst-strptime2.c rework logically
must follow the V4 strptime fix plus test additions, so, indeed, you were
right to suggest from the start that the test rework goes logically with the
strptime fix.

I will be posting a V5 series of two patches. The first is not significantly
changed from the V4 strptime fix plus test additions. The second updates
the tst-strptime2.c changes that I proposed.

Cheers,
James