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 <[email protected]> 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.
