Re: [PATCH] better diagnostics for seq

[Jim Meyering]

Steven Schubiger <address@hidden> wrote:
> Attached is a patch that enhances seq's diagnostics. If you agree
> that this is the right way to go, I'll amend other files (ChangeLog,
> etc.) as needed.

Thanks for working on that.

> diff --git a/src/seq.c b/src/seq.c
> index 261a44b..4a6f96e 100644
> --- a/src/seq.c
> +++ b/src/seq.c
> @@ -185,12 +185,38 @@ scan_arg (const char *arg)
>  static char const *
>  long_double_format (char const *fmt, struct layout *layout)
>  {
> +  const char *fmt_scan;
>    size_t i;
> +  size_t counted_directives = 0;
>    size_t prefix_len = 0;
>    size_t suffix_len = 0;
>    size_t length_modifier_offset;
>    bool has_L;
>
> +  fmt_scan = (const char *) fmt;
> +  while (*fmt_scan)
> +    {
> +      if (*fmt_scan == ' ')
> +        fmt_scan++;
> +      else
> +        {
> +          if (*fmt_scan == '%'
> +              && (*(fmt_scan + 1) != '%'
> +               && *(fmt_scan + 1) != ' '
> +               && *(fmt_scan + 1) != '\0'))
> +            {
> +              counted_directives++;
> +              fmt_scan += 2;
> +            }
> +          else
> +            fmt_scan++;
> +        }
> +    }
> +  if (counted_directives == 0)
> +    error (EXIT_FAILURE, 0, _("no %% directive"));

This might be a little more readable:

    error (EXIT_FAILURE, 0, _("no %% directive in format %s"), quote (fmt));

> +  else if (counted_directives > 1)
> +    error (EXIT_FAILURE, 0, _("too many %% directives"));
> +

I think you can simplify that.
How about something like this?

  unsigned int n_directives = 0;
  char const *p = fmt;
  for (p = fmt; *p; p++)
    {
      if (*p == '%')
        {
          if (p[1] != '%' && p[1] != '\0')
            {
              ++n_directives;
              ++p;
            }
        }
    }

Of course, it'd be nice to have a comment or two.
Even better: just add a new function to validate the format string,
including your two new diagnostics.  Then the standard function-
describing comment will suffice.

> -  if (! strchr ("efgaEFGA", fmt[i]))
> -    return NULL;
> +  if (! strchr ("efgaEFGA", fmt[i]))
> +    error (EXIT_FAILURE, 0, _("invalid directive: `%c%c'"), fmt[i - 1], 
> fmt[i]);

It's probably not worth a new test here,
since this is already handled by the caller.
The full directive may be many bytes long, so in general, %c%c is not
enough.  Also, please don't add literal `' quotes in diagnostics.
If you want to quote something, use one of the quote* functions.

Please add test cases that exercise the new code
and test for the expected (new) diagnostics.
Each new test case should be in the form of a new line or two
in the file, tests/misc/seq.

When testing for a diagnostic, use this one as a model:

   ['fmt-c',    qw(-f %%g 1), {EXIT => 1},
    {ERR => "seq: invalid format string: `%%g'\n"
     . "Try `seq --help' for more information.\n"},