coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: numfmt: minor improvements to --field parsing


From: Pádraig Brady
Subject: Re: numfmt: minor improvements to --field parsing
Date: Sun, 12 Jul 2015 19:33:29 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 11/07/15 01:58, Assaf Gordon wrote:
> Hello,
> 
> attached two small patches:
> ===
> Subject: [PATCH 1/2] numfmt: improve --field parsing, add tests
> 
> * src/numfmt.c: parse_field_arg() detect and fail on negative values
>    (previously, implicit conversion from signed strtol to unsigned size_t
>    failed to detect those); detect and fail on invalid ranges '1-2-3';
>    avoid adding superfluous range item for 'N-N' ranges.

Excellent. -Wconversion would have flagged this issue,
but it really is too awkward and flags many valid situations.
Worth trying to avoid with new code of course.

> * tests/misc/numfmt.pl: test above scenarios, add few more tests for
>    better coverage.

> +     ['field-range-err-1', '--field -foo --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid field value 'foo'\n"}],
> +     ['field-range-err-2', '--field --3 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid field value '-3'\n"}],
> +     ['field-range-err-3', '--field 0 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid field value '0'\n"}],
> +     ['field-range-err-4', '--field 3-2 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid decreasing range\n"}],
> +     ['field-range-err-5', '--field 1,-2 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid field value '-2'\n"}],

Hmm. Shouldn't we treat the above like field 1 and all fields up to 2 inclusive?
Similarly shouldn't we allow ranges like:

$ echo 1 2 3 4 5 | numfmt --field -2,4- --to-unit=1000 --round=down
0 0 3 0 0

$ echo 1 2 3 4 5 | numfmt --field -2,-4 --to-unit=1000 --round=down
0 0 0 0 5

Currently we get:
$ echo 1 2 3 4 5 | numfmt --field -2,4- --to-unit=1000 --round=down
0 0 3 4 5
$ echo 1 2 3 4 5 | numfmt --field -2,-4 --to-unit=1000 --round=down
0 0 3 4 5

> +     ['field-range-err-6', '--field - --field 1- --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: multiple field specifications\n"}],
> +     ['field-range-err-7', '--field -1 --field 1- --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: multiple field specifications\n"}],
> +     ['field-range-err-8', '--field -1 --field 1,2,3 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: multiple field specifications\n"}],
> +     ['field-range-err-9', '--field 1- --field 1,2,3 --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: multiple field specifications\n"}],
> +     ['field-range-err-10','--field 1,2,3 --field 1- --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: multiple field specifications\n"}],

Those look good.

> +     ['field-range-err-11','--field 1-2-3 --field 1- --to=si 10',
> +             {EXIT=>1}, {ERR=>"$prog: invalid field range\n"}],

nit '--field 1-' is redundant/confusing here

thanks!
Pádraig.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]