Re: [PATCH] Command line parsing of ls with genparse

[Jim Meyering]

Eric Blake-1 <address@hidden> wrote:
>> -    case 'w':
>> -      {
>> -        unsigned long int tmp_ulong;
>> -        if (xstrtoul (optarg, NULL, 0, &tmp_ulong, NULL) != LONGINT_OK
>> -            || ! (0 < tmp_ulong && tmp_ulong <= SIZE_MAX))
>> -          error (LS_FAILURE, 0, _("invalid line width: %s"),
>> -                 quotearg (optarg));
>
> [Side issue, not a problem with your patch, but your
> patch highlights the problem - why are we using plain
> error instead of xstrtol_error?  It means this particular
> error message loses some quality.]

You're right that technically xstrtol_error is better -- more
specific about why a string is invalid -- and I'll probably make
the change, but I can't help lamenting the small degradation in
quality of the English diagnostic:

Current:

    $ ls -w999999999999999999999999999999999999999
    ls: invalid line width: 999999999999999999999999999999999999999
    [Exit 2]

Probable new diagnostic:

    $ ./ls -w999999999999999999999999999999999999999
    ./ls: -w argument `999999999999999999999999999999999999999' too large
    [Exit 2]

As you probably noticed, it's needed at least in many places.
A quick search shows many other cases like that.

    $ grep -A5 -E 'xstrto(u?l|i?max) \(' *.c|grep -w error|wc -l
    32

Do you feel like working on that?