Re: Z and Y suffixes in xstrtol.c

[Pádraig Brady]

On 18/12/14 20:38, Pádraig Brady wrote:
> 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().

Actually a better idea since accepting octal/hex is the exception,
would be to maintain xdectoumax() and provide a new xnumtoumax()
which the former just calls.

p.s. Just set up spell check in vim.
It was unexpectedly easy, and I put this in my .vimrc:

"Note <leader> is the user modifier key (like g is the vim modifier key)
"One can change it from the default of \ using: let mapleader = ","
"\s to toggle spellcheck
nmap <silent> <leader>s :setlocal spell! spelllang=en_gb<CR>
"\u to toggle US spellcheck
nmap <silent> <leader>u :setlocal spell! spelllang=en_us<CR>

cheers,
Pádraig.