Re: Z and Y suffixes in xstrtol.c

[Pádraig Brady]

On 18/12/14 19:22, Jim Meyering wrote:
> On Thu, Dec 18, 2014 at 10:34 AM, Pádraig Brady <address@hidden> wrote:
>> On 16/12/14 17:05, Paul Eggert wrote:
>>> On 12/16/2014 05:46 AM, Pádraig Brady wrote:
>>>>           if (xstrtoul (optarg, NULL, 10, &val, "") != LONGINT_OK
>>>> -            || MIN (INT_MAX, SIZE_MAX) < val)
>>>> -          error (EXIT_FAILURE, 0, _("%s: invalid number"), optarg);
>>>> +            || (MIN (INT_MAX, SIZE_MAX) < val && (errno = EOVERFLOW)))
>>>> +          error (EXIT_FAILURE, errno, _("invalid number %s"), quote 
>>>> (optarg));
>>>
>>> I'm afraid I find this sort of code hard to read, due to the side effect
>>> withinn an 'if' expression.  The GNU coding standards say "Try to avoid
>>> assignments inside if-conditions (assignments inside while-conditions
>>> are ok)."
>>> <https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html> 
>>> Can
>>> you please redo the patch to avoid this sort of thing?  Perhaps with a
>>> wrapper around xstrtoul?  Thanks.
>>
>> In the attached I used an xdectoumax() wrapper to handle
>> the common case of parsing a positive bounded integer
>> and exiting with a diagnostic on error.
>>
>> A net removal of 60 lines, but more importantly
>> it simplifies a lot of bounds checking which is error prone.
> 
> Very nice!
> Nit in commit log message: s/exiiting/exiting/
> 
> Nit in comment: s/Use have/Use /:
> 
>> +          /* Use have the INT range as a heuristic to distinguish
>> +             type overflow rather than other min/max limits.  */
> 
> Do you want to add a "base" parameter to this API, since some existing
> uses of xstrtoul pass "0" as the base (to allow octal and hexadecimal),
> while others pass "10", to allow only decimal strings?
> 
> If no existing test fails with your change, this exposes a lack
> in our coverage of that admittedly small corner.

Good catch, yes I'll need to augment the tests for that.
ls, shred -s, and stty accept octal and hex :/
While that's inconsistent, we don't want to regress here of course.
Accepting a base  breaks the "dec" naming so I may adjust to
something like xnumtoumax().

thanks,
Pádraig.