Switching to [KMGTPE]i prefixes?

[Xin LI]

On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best <arundel at freebsd.org> wrote:
> On Fri Mar 25 11, Xin LI wrote:
>> FYI I have a patch and I have incorporated some of Alexander's idea.
>>
>> Difference:
>>
>>  - Use of both HN_DIVISOR_1000 and HN_IEC_PREFIXES triggers an
>> assertion.  I think it doesn't make sense to return since this is an
>> API violation and we should just tell the caller explicitly;
>
> actually i vote for removing all asserts in humanize_number.c and return -1
> based upon the later checks.
>
> the existing
>
> assert(buf != NULL);
> assert(suffix != NULL);
>
> checks aren't really needed, since buf and suffix are also checked later on. so
> just having:

Well, one of my believes is that a program should crash as early as
possible, and with clear statement about "Why I crashed", when it's
compiled with debugging aids, like assertions.  To test or not to test
these cases in a release binary on the other hand really depends on
whether there is security or other bad implications.  This generally
makes developers' life easier, as they don't have to pursue into the
code to find out why the program crashed, etc.

Unlike system calls, humanize_number(3) does not indicate what's wrong
via errno, e.g. it tells you "No I can't" rather than a reason of "Why
I can't do that".  Assertions here gives it an opportunity to say it
loudly.

If however the program is compiled with -DNDEBUG, these assertions
would became no-op.  At this stage, in my opinion, only basic tests
should be done and we fall back to returning -1, or at least, not
crash the program in a mysterious way.

For this reasons, I think the assertion here against flags is right,
it does not hurt if we proceed with both flags set, as we do not
access undefined memory nor overwrite undefined memory.  Furthermore,
these values are more likely to be hard-wired at caller, where the
assertion should catch the case.

>        if (scale <= 0 || (scale >= maxscale &&
>            (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0))
>                return (-1);

I think this one is good to have for both assertion and tests.  Note
that I think it should be scale < 0 here, scale == 0 seems to be a
valid value.

>        if (buf == NULL || suffix == NULL)
>                return (-1);

This duplication is necessary in my opinion.  It's a protection
against NULL pointer deference at runtime.

>        if ((flags & (HN_DIVISOR_1000 | HN_IEC_PREFIXES)) == 0)
>                return (-1);

I'd vote no for this one for the reason above.

>>  - DIVISOR_1000 and !1000 cases use just same prefixes, so merge them
>> while keeping divisor intact;
>
> good idea. however i think you should add a comment to point out that the
> default behavior is !DIVISOR_1000 && !HN_IEC_PREFIXES. one has to look very
> closely to find out.

Will do.

> #define HN_DECIMAL              0x01
> #define HN_NOSPACE              0x02
> #define HN_B                    0x04
> #define HN_DIVISOR_1000         0x08
> #define HN_IEC_PREFIXES         0x40
>
> #define HN_GETSCALE             0x10
> #define HN_AUTOSCALE            0x20

Thinking again and I think we are just fine to use HN_IEC_PREFIXES ==
0x10 here.  I don't think there is a reason why we can't use 0x10 for
flags.

Here is what in my mind.  I have stolen some comments from your
version of patch to explain the meaning of the HN_IEC_PREFIXES option
as well.

-- 
Xin LI <delphij at delphij.net> http://www.delphij.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: humanize_number.c.diff
Type: text/x-patch
Size: 2910 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20110325/3eeb9235/humanize_number.c-0001.bin