Re: [PATCH] time/tst-strptime2.c: test full input range -9999 to +9999

[James Perkins <james at loowit dot net>]

Thanks for your patient and prompt review, Mike.

I'm putting my tst-strptime2.c rework into a patch following the one that
repairs strptime_l.c's %z issues, and calling the patchset V5 - I shall post
it momentarily.

On Thu, Aug 13, 2015 at 6:29 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 13 Aug 2015 17:49, James Perkins wrote:
>> +/*
>> +   Write a string into the supplied buffer, containing a dummy string,
...
>> +   part of the time is outside the valid range of 00 to 59.
>> +   */
>
> GNU style says the first & last lines should be cuddled.  e.g.
> /* Write a string ....
>    part of the time is outside the valid range of 00 to 59.  */
>
>> +     sprintf(buf, "%s %c", dummy_string, sign);
>
> GNU style says there should be a space before the (

I've addressed the GNU C style issues you raised, and ran my code through
indent, which fixed up a few more.

>> +      printf ("%s: tm_gmtoff is %ld\n", buf, (long int) tm.tm_gmtoff);
>
> tm_gmtoff is already long int, so why the cast ?  i know it was there before,
> but the question remains.

I'm not sure why this cast was here historically. I will remove it -- as you
point out, it is unnecessary.

>> +static int
>> +do_test (void)
>
> this could do with a --verbose option i think that'd dump every buffer tested
> and the results of the test.  trusting silent output does the right thing vs
> getting a verbose report and scanning by eye/grep/whatever makes me way more
> confident and helps check for future regressions.
>
> look at the CMDLINE_OPTIONS & CMDLINE_PROCESS defines in test-skeleton.c.

That is a great suggestion; I had been adding verbose code and then removing
it prior to sending the patch, and this is a way that it can stay and remain an
aid to people debugging strptime behavior. I've worked that in, too.

Cheers,
James