Switching to [KMGTPE]i prefixes?

[Alexander Best]

On Fri Mar 25 11, Xin LI wrote:
> 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.

any plans to commit this patch?

> 
> -- 
> Xin LI <delphij at delphij.net> http://www.delphij.net



-- 
a13x