Re: Z and Y suffixes in xstrtol.c

[Jim Meyering]

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.