Date: Thu, 27 Dec 2012 08:31:36 -1000 From: Clifton Royston <cliftonr@volcano.org> To: Eitan Adler <lists@eitanadler.com>, freebsd-hackers@freebsd.org, John-Mark Gurney <jmg@funkthat.com> Subject: Re: looking for someone to fix humanize_number (test cases included) Message-ID: <20121227183136.GA77509@volcano.org> In-Reply-To: <20121226205307.GA66528@volcano.org> References: <mailman.7.1356523201.47461.freebsd-hackers@freebsd.org> <20121226205307.GA66528@volcano.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Dec 26, 2012 at 10:53:07AM -1000, Clifton Royston wrote:
> On Wed, Dec 26, 2012 at 12:00:01PM +0000, freebsd-hackers-request@freebsd.org wrote:
> > Date: Tue, 25 Dec 2012 14:52:09 -0500
> > From: Eitan Adler <lists@eitanadler.com>
> > To: freebsd-hackers@freebsd.org, John-Mark Gurney <jmg@funkthat.com>
> > Subject: Re: looking for someone to fix humanize_number (test cases included)
> > Message-ID:
> > <CAF6rxgkCOdg2Ep2pdxJKjCyQzbYNrE_tPT3CqeygwrtZ6AkVSw@mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > On 25 December 2012 14:46, Clifton Royston <cliftonr@volcano.org> wrote:
> > >> I correct myself: the function works fine, and there are no bugs I
> > > could find, though it's clear the man page could emphasize the correct
> > > usage a bit more.
> >
> > Can you submit a diff to the man page as well? I figure if you got
> > confused at least 10 others got even more confused.
>
> I'd be happy to, and will do so soon.
...
Update - I've much expanded the test cases, up to around 120 so far,
although they don't yet cover HN_GETSCALE, HN_DECIMAL, HN_NOSPACE, or
HN_B. I may put in some rudimentary tests for HN_GETSCALE but don't
plan to cover the latter three at all, although HN_NOSPACE and HN_B are
pretty trivial. I've also added checking of expected vs. actual
return values. I'll put the updated test program somewhere shortly.
In the current library routine, these tests have found one more piece
of arguable misdesign for discussion, and several out-and-out bugs.
1) Possible misdesign - the current implementation raises an assertion
and coredumps the calling program on a negative scale value; other
invalid scale values simply return -1. I'd argue both should do the
same thing, and I think the latter is less astonishing. (It's not like
passing a null buffer pointer IMHO.) It's not a huge issue though so
I'd be willing to leave it. The new test program has an option to
enable testing of negative scale values.
2) Bug: If compiled with NDEBUG to disable the assert, negative values
of scale < 0 would not be checked for. I am fixing this.
3) Bug: the only valid scale values are 0-7, HN_GETSCALE, and
HN_AUTOSCALE; no combinations of these are valid. (0 does no scaling
and acts as a glorified printf, so is rarely useful.) However, any
number which happens to have the HN_GETSCALE and/or HN_AUTOSCALE bit
set will be accepted and likely do something astonishing for the
caller. I am fixing this so only the specified values are accepted.
4) Documentation: needs to specify what I just wrote about scale
values, plus what must be true to avoid assertion failures, as well as
points raised above about values passed via scale vs. flags parms.
5) Bug: number values in the range of .5 exa[byte] and above are never
or rarely handled correctly; the code does some multiplies up front
which will cause them to wrap and return completely bogus values. I
haven't tried to fix this as yet. The new test program has a -E option
to enable testing of exabyte range number values. Obviously nobody's
using our utils on exabyte-sized filesystems as yet... The new test
program has a -E option to enable testing of exabyte range number
values.
6) Undefined case: when the function exits due to an invalid scale
value, it does not set the buffer (if valid) to an empty string. I
intend to have it do that, mostly to make it more convenient for
testing.
-- Clifton
Comparison of results from libutil vs. my current version:
[cliftonr@oz ~/humanize_number]$ ./test_humanize_numbers -E
wrong return value on index 6, got: 19 + "-53", expected 3 + "1 E"; num = 500000000000000000, scale = 32, flags= HN_DIVISOR_1000.
result mismatch on index 13, got: "0 E", expected "2 E"; num = 1500000000000000000, scale = 32, flags= HN_DIVISOR_1000.
result mismatch on index 20, got: "0 E", expected "1 E"; num = 576460752303423488, scale = 32, flags= .
result mismatch on index 27, got: "0 E", expected "2 E"; num = 1729382256910270464, scale = 32, flags= .
result mismatch on index 48, got: "0 E", expected "1 E"; num = 500000000000000000, scale = 6, flags= HN_DIVISOR_1000.
result mismatch on index 60, got: "0 E", expected "2 E"; num = 1500000000000000000, scale = 6, flags= HN_DIVISOR_1000.
result mismatch on index 72, got: "0 E", expected "1 E"; num = 576460752303423488, scale = 6, flags= .
result mismatch on index 84, got: "0 E", expected "2 E"; num = 1729382256910270464, scale = 6, flags= .
result mismatch on index 85, got: "0 E", expected ""; num = 1, scale = 7, flags= .
result mismatch on index 86, got: "0 E", expected ""; num = 1, scale = 7, flags= HN_DIVISOR_1000.
expected error on index 87, but got: 2 + "1 "; num = 1, scale = 1000, flags= .
expected error on index 88, but got: 2 + "1 "; num = 1, scale = 1000, flags= HN_DIVISOR_1000.
result mismatch on index 89, got: "1 ", expected ""; num = 0, scale = 1000000, flags= .
result mismatch on index 90, got: "1 ", expected ""; num = 0, scale = 1000000, flags= HN_DIVISOR_1000.
expected error on index 91, but got: 0 + ""; num = 0, scale = 2147483647, flags= .
expected error on index 92, but got: 0 + ""; num = 0, scale = 2147483647, flags= HN_DIVISOR_1000.
total errors: 16/117 tests
[cliftonr@oz ~/humanize_number]$ ./test_humanize_numbers_new -E -n
wrong return value on index 6, got: 19 + "-53", expected 3 + "1 E"; num = 500000000000000000, scale = 32, flags= HN_DIVISOR_1000.
result mismatch on index 13, got: "0 E", expected "2 E"; num = 1500000000000000000, scale = 32, flags= HN_DIVISOR_1000.
result mismatch on index 20, got: "0 E", expected "1 E"; num = 576460752303423488, scale = 32, flags= .
result mismatch on index 27, got: "0 E", expected "2 E"; num = 1729382256910270464, scale = 32, flags= .
result mismatch on index 48, got: "0 E", expected "1 E"; num = 500000000000000000, scale = 6, flags= HN_DIVISOR_1000.
result mismatch on index 60, got: "0 E", expected "2 E"; num = 1500000000000000000, scale = 6, flags= HN_DIVISOR_1000.
result mismatch on index 72, got: "0 E", expected "1 E"; num = 576460752303423488, scale = 6, flags= .
result mismatch on index 84, got: "0 E", expected "2 E"; num = 1729382256910270464, scale = 6, flags= .
total errors: 8/123 tests
--
Clifton Royston -- cliftonr@iandicomputing.com / cliftonr@volcano.org
President - I and I Computing * http://www.iandicomputing.com/
Custom programming, network design, systems and network consulting services
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121227183136.GA77509>