Switching to [KMGTPE]i prefixes?
Xin LI
delphij at gmail.com
Wed Mar 30 00:15:48 UTC 2011
On Tue, Mar 29, 2011 at 2:51 PM, Alexander Best <arundel at freebsd.org> wrote:
> 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?
I think I should be able to commit a final version by Friday, still
need some polishing...
Cheers,
--
Xin LI <delphij at delphij.net> http://www.delphij.net
More information about the freebsd-hackers
mailing list