looking for someone to fix humanize_number (test cases included)

[John-Mark Gurney]

Clifton Royston wrote this message on Tue, Dec 25, 2012 at 09:46 -1000:
> On Tue, Dec 25, 2012 at 08:23:55AM -1000, Clifton Royston wrote:
> > On Tue, Dec 25, 2012 at 07:20:37AM -1000, Clifton Royston wrote:
> > > On Mon, Dec 24, 2012 at 12:00:01PM +0000, freebsd-hackers-request at freebsd.org wrote:
> > > > From: John-Mark Gurney <jmg at funkthat.com>
> > > > To: hackers at FreeBSD.org
> > > > Subject: looking for someone to fix humanize_number (test cases
> > > > 	included)
> > > > 
> > > > I'm looking for a person who is interested in fixing up humanize_number.
> > ...
> > > > So I decided to write a test program to test the output, and now I'm even
> > > > more surprised by the output...  Neither 7.2-R nor 10-current give what
> > > > I expect are the correct results...
> > ...
> > 
> >   I am bemused.
> 
>   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.
> 
>   I had to read the source several times and start on debugging it
> before I understood the correct usage of the flag values with the scale
> and flags parameters, despite the man page stating:
> 
>      The following flags may be passed in scale:
> 
>        HN_AUTOSCALE     Format the buffer using the lowest multiplier pos-
>                         sible.
>        HN_GETSCALE      Return the prefix index number (the number of
>                         times number must be divided to fit) instead of
>                         formatting it to the buffer.
> 
>      The following flags may be passed in flags:
> 
>        HN_DECIMAL       If the final result is less than 10, display it
>                         using one digit.
> ...
>        HN_DIVISOR_1000  Divide number with 1000 instead of 1024.
> 
>   That is, certain flags must be passed in flags and others must only
> be passed in scale - a bit counter-intuitive.  Also, scale == 0 is
> clearly not interpreted as AUTOSCALE, but I am not yet clear how it is
> being handled - it seems somewhat like AUTOSCALE but not identical.
> 
>   When the test program constant table is updated to pass the scale
> flags as specified, as well as fixing the bugs mentioned in the
> previous emails, it all passes except for the one (intentional?)
> inconsistency that "k" is used in place of "K" if HN_DECIMAL is
> enabled.
> 
>   The bug in the transfer speed results which prompted this inquiry
> suggests that perhaps some clients of humanize_number in the codebase
> are also passing the scale parameters incorrectly.  I would propose
> accepting HN_AUTOSCALE and HN_GETSCALE in the flags field (they don't
> overlap with other values) while continuing to accept them in the scale
> field for backwards compatibility.  Trivial diff below.

Sorry I didn't get back to this, but now I have a few minutes...

> +	getscale  = (flags | scale) & HN_GETSCALE;

This isn't good:
#define HN_IEC_PREFIXES         0x10
#define HN_GETSCALE             0x10

If someone sets HN_IEC_PREFIXES, they'll acidentally enable _GETSCALE..

We could do something anoying by changing the value of _GETSCALE, and
then leaving some legacy code to accept the old _GETSCALE on the scale
input...  This would let new code work, but would break new code on
old libraries...  So, I don't see an easy way to fix this...

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."