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

[James Perkins <james at loowit dot net>]

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