Re: numfmt: minor improvements to --field parsing

[Pádraig Brady]

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.