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.
