[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Z and Y suffixes in xstrtol.c
From: |
Pádraig Brady |
Subject: |
Re: Z and Y suffixes in xstrtol.c |
Date: |
Thu, 18 Dec 2014 20:38:19 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 |
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.