[erlang-questions] new float_to_list/2 BIF - critique wanted

[Gleb Peregud]

Mikael, unfortunately you've checked the previous version of my patch.
I've reimplemented it using asterix precision argument.

More comments inline:

On Mon, May 4, 2009 at 15:52, Mikael Pettersson <mikpe@REDACTED> wrote:
> Gleb Peregud writes:
>  > +     if (is_not_float(BIF_ARG_1))
>  > +     BIF_ERROR(BIF_P, BADARG);
> indentation?

It is 1-to-1 copy from the original sys_double_to_chars(double,char)
used by float_to_list/2. When viewing in `emacs` or `less` this
indentation is coherent with the whole file. Though myself I'd prefer
the version which does not mix tabs and spaces...

>  > +     if (is_not_small(BIF_ARG_2) || unsigned_val(BIF_ARG_2) > 250)
>  > +         BIF_ERROR(BIF_P, BADARG);
>  > +     GET_DOUBLE(BIF_ARG_1, f);
>  > +     precision = unsigned_val(BIF_ARG_2);
> precision being <= 250 should be 'int' or 'unsigned int' not 'Sint'

It was done based on the analogy with surrounding code e.g.
make_tuple_2. Sint is short integer, right? New version of patch has a
precision limit of 500 digits. Why? It is just arbitrary number, which
fits into buffer of 512 chars.

>  > +     if ((i = sys_double_to_chars_precision(f.fd, precision, fbuf)) <= 0)
> please don't mix assignments with tests; just write the obvious

It is copied from the original sys_double_to_chars(double,char) used
by float_to_list/2. I'll fix it.

> put an empty line between the declarations and the first statement

Got that! I'll fix it.

> why not just call sprintf(buf, "%.*e", precision, fp)?

It is fixed in the newer patch.

>  > +    /* Search upto decimal point */
>  > +    if (*s == '+' || *s == '-') s++;
>
>  > +    while (ISDIGIT(*s)) s++;
>
>  > +    if (*s == ',') *s++ = '.'; /* Replace ',' with '.' */
>
>  > +    /* Scan to end of string */
>  > +    while (*s) s++;
>
>  > +    return s-buf; /* i.e strlen(buf) */
>
> again, why not just call strlen()?

All this is copied from the original sys_double_to_chars(double,char).
Here is the original comment for the sys_double_to_chars(double,
char):

/*
 ** Convert a double to ascii format 0.dddde[+|-]ddd
 ** return number of characters converted
 **
 ** These two functions should maybe use localeconv() to pick up
 ** the current radix character, but since it is uncertain how
 ** expensive such a system call is, and since no-one has heard
 ** of other radix characters than '.' and ',' an ad-hoc
 ** low execution time solution is used instead.
 */

I guess my intent and original author's are the same here, so I'd
prefer to leave it as is.

> why is this different from the Unix and Win32 versions?

It is done in analogy to sys_double_to_chars(double, char). I guess
VXWorks always uses "." as a separator...

Many thanks for your remarks!



> Hmm, I was under the impression that no one in their right mind would
> use floats for financial stuff. Or do You mean something that is financial
> but not money?

I guess that Per needed this function to represent a lot of financial
data in text-based representation - e.g. sending it via JSON/XML.