C Implementation of atof

Question

I'm a beginner at C. I'm currently implementing atof to build a raytracer, however I'm still learning how to efficiently write programs.

Assignement

Instructions

The program takes a scene description file as argument to generate objects. Some of these paramaters are float. Example of the file.

I'm parsing through the file. As I'm restricted in terms of lines allowed per function and I'm currently learning how double pointers work, I'm using a double char pointer. Example of one such function using lc_atof.

int    a_parsing(char *str, t_pars *data)
{
    if (*(str++) == 'A')
    {
        if (((data->a_ratio = lc_atof(&str)) >= 0.0) && data->a_ratio <= 1.0 && errno == 0)
        // 
            if (((data->a_R = lc_atoi(&str)) >= 0) && data->a_R <= 255 && errno == 0)
                if (*(str++) = ',' && ((data->a_G = lc_atoi(&str)) >= 0) && data->a_G <= 255 && errno == 0)
                    if (*(str++) = ',' && ((data->a_B = lc_atoi(&str)) >= 0) && data->a_B <= 255 && errno == 0)
                        return (skip_space(&str));
    }
    return (0);
}

Current code to review:

#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <limits.h>
#include <errno.h>
#include <float.h>

static float    conversion(char **str)
{
    double    d_nbr;
    double    power;

    d_nbr = 0.0;
    power = 10.0;
    while (isdigit(**str))
    {
        d_nbr = d_nbr * 10.0 + (**str - 48);
        if (d_nbr > FLT_MAX)
        {
            errno = EIO;
            return (-1);
        }
        (*str)++;
    }
    if (**str == '.')
    {
      (*str)++;
      if (isdigit(**str))
      {
        d_nbr = d_nbr * 10.0 + (**str - 48);
        if (d_nbr > FLT_MAX)
        {
            errno = EIO;
            return (-1);
        }
        (*str)++;
        return ((float)(d_nbr / power));
      }
    }
    errno = EIO;
    return (-1);
}

float            lc_atof(char **str)
{
    float    n;
    int        sign;

    n = 0.0;
    sign = 1.0;
    if (!str || !*str)
    {
        errno = EIO;
        return (-1);
    }
    while (isspace(**str))
        (*str)++;
    if (**str == '+' || **str == '-')
    {
        if (**str == '-')
            sign = -1.0;    
        (*str)++;
    }
    if (!isdigit(**str))
    {
        errno = EIO;
        return (-1);
    }
    if ((n = conversion(str)) == 0 && errno != 0)
       return (-1);
    return (sign * n);
}

The only tweaks to the actual atof I've made is having a double char pointer as argument and returning -1 in case of errors.

Every input much appreciated.

Answer 1

Portability

Nie ma gwarancji, że ten kod będzie używał ASCII, więc lepiej byłoby użyć '0'zamiast tego, 48który jest czymś w rodzaju magicznej liczby. Używanie '0'sprawia, że ​​jest bardziej czytelny i łatwiejszy do zrozumienia.

lc_atof Nie obsługuje poprawnie zakończenia łańcucha lub końca wiersza

Ten kod nie obsługuje łańcucha zakończonego znakiem NULL ani znaku końca wiersza. Funkcja isspace()zwraca truekoniec linii, więc kod przejdzie tuż obok niej.

    while (isspace(**str))
        (*str)++;
    if (**str == '+' || **str == '-')
    {
        if (**str == '-')
            sign = -1.0;
        (*str)++;
    }
    if (!isdigit(**str))
    {
        errno = EIO;
        return (-1);
    }

Złożoność

Rozumiem, że nie prosiłeś o przegląd, ale złożoność każdej ifinstrukcji w przykładowej funkcji wywołania jest zbyt duża i spowodowała, że ​​popełniłem błąd w mojej poprzedniej recenzji:

int a_parsing(char* str, t_pars* data)
{
    if (*(str++) == 'A')
    {
        if (((data->a_ratio = lc_atof(&str)) >= 0.0) && data->a_ratio <= 1.0 && errno == 0)
            // 
            if (((data->a_R = lc_atoi(&str)) >= 0) && data->a_R <= 255 && errno == 0)
                if (*(str++) = ',' && ((data->a_G = lc_atoi(&str)) >= 0) && data->a_G <= 255 && errno == 0)
                    if (*(str++) = ',' && ((data->a_B = lc_atoi(&str)) >= 0) && data->a_B <= 255 && errno == 0)
                        return (skip_space(&str));
    }
    return (0);
}

Przepisałbym kod jako:

#define MAX_COLOR   0xFF
int a_parsing_prime(char* str, t_pars* data)
{
    if (*(str++) == 'A')
    {
        data->a_ratio = lc_atof(&str);
        if (!errno && data->a_R <= MAX_COLOR)
        {
            if (*(str++) = ',')
            {
                data->a_G = lc_atoi(&str);
                if (!errno && data->a_G <= MAX_COLOR)
                {
                    if (*(str++) = ',')
                    {
                        data->a_B = lc_atoi(&str);
                        if (!errno && data->a_B <= MAX_COLOR)
                        {
                            return (skip_space(&str));
                        }
                    }
                }
            }
        }
    }
    return (0);
}

co naprawdę pokazuje złożoność funkcji.

Answer 2

• Choosing EIO for error reporting is very dubious. lc_atof doesn't do any input or output; why should it report IO error? If the return type cannot represent the result (e.g. d_nbr > FLT_MAX), a logical choice is ERANGE or EOVERFLOW. If the conversion cannot complete because of the malformed argument (e.g. !isdigit(**str)), the logical choice would perhaps be EINVAL.

  That said, I do not endorse setting errno in the library function. A long standing tradition is to set errno in system calls only. I know that this tradition is more and more violated these days, but still. If you have other means of error reporting, stick with them.

• Using inout parameter (str in your case) is not advisable. It unnecessarily complicates the code, both on the caller side and the callee side. The callee is forced to use extra indirection too many times, and to worry about parenthesizing (**str)++. In turn, the caller loses track on where the parsable began (say, it needs to log the malformed number). Look how strtof handles this:

    float strtof(const char *restrict nptr, char **restrict endptr);
  

  Here nptr is an in-only, and endptr is out-only.

• I am surprised that you decided to limit the utility of the function by handling only one digit after the decimal dot. It is not a big effort to handle all of them, and the benefits are much greater.

• There is no need to parenthesize the return value. return is an operator, not a function.