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

[Mikael Pettersson]

Gleb Peregud writes:
 > +BIF_RETTYPE float_to_list_2(BIF_ALIST_2)
 > +{
 > +     int i;
 > +     Sint precision;
 > +     Uint need;
 > +     Eterm* hp;
 > +     FloatDef f;
 > +     char fbuf[512];
 > +     
 > +     /* check the arguments */
 > +     if (is_not_float(BIF_ARG_1))
 > +	 BIF_ERROR(BIF_P, BADARG);

indentation?

 > +     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'

 > +     if ((i = sys_double_to_chars_precision(f.fd, precision, fbuf)) <= 0)

please don't mix assignments with tests; just write the obvious

       i = sys_double_...(...);
       if (i <= 0)

 > diff --git a/erts/emulator/sys/unix/sys_float.c b/erts/emulator/sys/unix/sys_float.c
 > index 0285678..e2d2bbf 100644
 > --- a/erts/emulator/sys/unix/sys_float.c
 > +++ b/erts/emulator/sys/unix/sys_float.c
 > @@ -726,6 +726,22 @@ sys_double_to_chars(double fp, char *buf)
 >      return s-buf; /* i.e strlen(buf) */
 >  }
 >  
 > +int
 > +sys_double_to_chars_precision(double fp, Sint precision, char *buf)
 > +{
 > +    char *s = buf;
 > +    char prec[16];

put an empty line between the declarations and the first statement

 > +    (void) sprintf(prec, "%%.%de", (int)precision);
 > +    (void) sprintf(buf, prec, fp);

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

 > +    /* Search upto decimal point */
 > +    if (*s == '+' || *s == '-') s++;

the then statement should be on new line + indentation

 > +    while (ISDIGIT(*s)) s++;

ditto for loop body

 > +    if (*s == ',') *s++ = '.'; /* Replace ',' with '.' */

ditto

also, why not just call strchr(buf, ',')?

 > +    /* Scan to end of string */
 > +    while (*s) s++;

new line + indentation for the loop body

 > +    return s-buf; /* i.e strlen(buf) */

again, why not just call strlen()?

 > +}
 > +
 >  /* Float conversion */
 >  
 >  int
 > diff --git a/erts/emulator/sys/vxworks/sys.c b/erts/emulator/sys/vxworks/sys.c
 > index abddc7e..faba284 100644
 > --- a/erts/emulator/sys/vxworks/sys.c
 > +++ b/erts/emulator/sys/vxworks/sys.c
 > @@ -1595,6 +1595,14 @@ int sys_double_to_chars(double fp, char *buf)
 >    return strlen(buf);
 >  }
 >  
 > +int
 > +sys_double_to_chars_precision(double fp, Sint precision, char *buf)
 > +{
 > +    char prec[16];
 > +    (void) sprintf(prec, "%%.%de", (int)precision);
 > +    (void) sprintf(buf, prec, fp);
 > +    return strlen(buf);
 > +}

why is this different from the Unix and Win32 versions?