This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] time/tst-strptime2.c: test full input range -9999 to +9999
- 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>
- Date: Sat, 15 Aug 2015 11:56:36 -0700
- Subject: Re: [PATCH] time/tst-strptime2.c: test full input range -9999 to +9999
- Authentication-results: sourceware.org; auth=none
- References: <1439513365-24723-1-git-send-email-james at loowit dot net> <55CDFED3 dot 70004 at redhat dot com>
On Fri, Aug 14, 2015 at 7:44 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/13/2015 08:49 PM, James Perkins wrote:
>> 2015-08-13 James Perkins <james@loowit.net>
>>
>> * time/tst-strptime2.c (tests): Replace short list of test
>> strings for strptime %z specifier with code that calculates
>> every combination of digits. Add tests for 0, 1, and 3 digit
>> invalid strings, which strptime correctly rejects.
>>
>
> This test looks great to me. I think the following next steps are
> required:
>
> - Add a verbose option.
> - Fixup GNU-style formatting issues.
> - Repost fix + test case changes as "v3"
> - I'll do a final review and checkin.
Carlos, thanks for the review. In the upcoming V5 patchset I've done what you've
asked for.
>> static int
>> -do_test (void)
>> +mkbuf (char *buf, int hhmm_signed, size_t ndigits)
>> {
>> - int result = 0;
>> + const int mm_max = 59;
>> + int neg = hhmm_signed < 0 ? 1 : 0;
>> + unsigned int hhmm = neg ? -hhmm_signed : hhmm_signed;
>> + unsigned int hh = hhmm / 100;
>> + unsigned int mm = hhmm % 100;
>> + char sign = neg ? '-' : '+';
>> + int gmtoff_signed = LONG_MAX;
>
> Looks good. It's probably simplest to just use LONG_MAX like this than
> anything more complicated.
By the way, I noticed my iteration from -9959 to 9959 was getting its sign byte
by looking at the value of the two's-complement int argument to mkbuf().
This meant my tests were missing the -0000, -000, -00, -0 and - input string
cases. I changed mkbuf to pass in sign information in a separate argument
so that it now tests the negative 0 cases as well.
>> - for (int i = 0; i < ntests; ++i)
>> + switch (ndigits)
>> {
>> - struct tm tm;
>> + case 0:
>> + sprintf(buf, "%s %c", dummy_string, sign);
>> + break;
>>
>> - if (strptime (tests[i].fmt, "%s %z", &tm) == NULL)
>> - {
>> - if (tests[i].gmtoff != LONG_MAX)
>> - {
>> - printf ("round %d: strptime unexpectedly failed\n", i);
>> - result = 1;
>> - }
>> - continue;
>> - }
>> + case 1:
>> + case 2:
>> + sprintf(buf, "%s %c%0*u", dummy_string, sign, ndigits, hh);
>> + break;
>> +
>> + case 3:
>> + case 4:
>> + sprintf(buf, "%s %c%0*u", dummy_string, sign, ndigits, hhmm);
>> + break;
>> +
>> + default:
>> + sprintf(buf, "%s %c%u", dummy_string, sign, hhmm);
>> + break;
>> +
>> + }
I've simplifed the above to just print the string using sprint, including
all four digits, and then truncate the string to the correct length using
ndigits. This simplifies logic throughout greatly.
>> +static int
>> +do_test (void)
>> +{
>> + const int hhmm_min = -9999;
>> + const int hhmm_max = 9999;
>> + const int hh_min = -99;
>> + const int hh_max = 99;
>> + int result = 0;
>> + int gmtoff;
>> + char buf[buffer_size];
>> +
>> + /* Test sign missing, four digits string (invalid format). */
>> +
>> + sprintf(buf, "%s 1030", dummy_string);
>> + gmtoff = LONG_MAX;
>> + result |= compare(buf, gmtoff);
>> +
>> + /* Test sign and no digits strings (invalid format). */
>> +
>> + gmtoff = mkbuf(buf, 0, 0);
>> + result |= compare(buf, gmtoff);
>> +
>> + /* Test sign and one digit strings (invalid format). */
>> +
>> + for (int hh = hh_min / 10; hh <= hh_max / 10; hh++)
>> + {
>> + gmtoff = mkbuf(buf, hh * 100, 1);
>> + result |= compare(buf, gmtoff);
>> + }
>> +
>> + /* Test sign and two digits strings (valid format). */
>> +
>> + for (int hh = hh_min; hh <= hh_max; hh++)
>> + {
>> + gmtoff = mkbuf(buf, hh * 100, 2);
>> + result |= compare(buf, gmtoff);
>> + }
>> +
>> + /* Test sign and three digits strings (invalid format). */
>> +
>> + for (int hhmm = hhmm_min / 10; hhmm <= hhmm_max / 10; hhmm++)
>> + {
>> + gmtoff = mkbuf(buf, hhmm, 1);
>> + result |= compare(buf, gmtoff);
>> + }
>> +
>> + /* Test sign and four digits strings. This includes cases
>> + where minutes are in the range 0 to 59 (valid), and
>> + where minutes are in the range 60 to 99 (invalid). */
>> +
>> + for (int hhmm = hhmm_min; hhmm <= hhmm_max; hhmm++)
>> + {
>> + gmtoff = mkbuf(buf, hhmm, 4);
>> + result |= compare(buf, gmtoff);
>> + }
>
> Looks great.
I am able to turn most of the above into nested for loops:
for ndigits from 0 to 4
for hhmm from 0 to 9999 by appropriate step based on ndigits
for sign from positive to negative
gmtoff = mkbuf(buf, hhmm, ndigits);
result |= compare (buf, gmtoff)
... making it much easier to read and clearer.
Patch set forthcoming...
Cheers,
James