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(). thanks, Pádraig.
