9
\$\begingroup\$

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.

Scene Description 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.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ You only process a single digit after the dot. Is it intended? \$\endgroup\$ – vnp Aug 20 '20 at 18:12
  • \$\begingroup\$ @vnp Yes it is. \$\endgroup\$ – lcols19 Aug 20 '20 at 18:18
  • 1
    \$\begingroup\$ Related: `atof` implementation \$\endgroup\$ – Peter Cordes Aug 21 '20 at 9:54
  • 1
    \$\begingroup\$ Perhaps you could explain why you're writing your own scanner, instead of using the facilities already provided by e.g. scanf, or doing the scanner in (f)lex? \$\endgroup\$ – jamesqf Aug 21 '20 at 16:58
  • \$\begingroup\$ @jamesqf One of the side goals of my assignement is to learn how library functions work by retyping them. \$\endgroup\$ – lcols19 Aug 21 '20 at 17:10
10
\$\begingroup\$

Portability

There is no guarantee that this code will be using ASCII so it would be better to use '0' rather than 48 which is something of a magic number. Using '0' makes it more readable and easier to understand.

lc_atof Doesn't Handle String Termination or End Of Line Correctly

This code doesn't handle a NULL terminated string or an end of line character. The function isspace() returns true for end of line so the code will walk right past it.

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

Complexity

I reaize that you didn't ask for this to be review, but the complexity of each if statement in the example call function is too much and caused me to make an error in my review previously:

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);
}

I would rewrite the code as :

#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);
}

which truely shows the complexity of the function.

\$\endgroup\$
6
  • 3
    \$\begingroup\$ NULL is a pointer constant. Perhaps you meant ASCII NUL ('\0') as the string terminator? IMO it's clearer to just say 0-terminated. \$\endgroup\$ – Peter Cordes Aug 21 '20 at 9:58
  • 1
    \$\begingroup\$ @PeterCordes I'm don't think it's any less clear, or rarely used; I see null-terminated everywhere in the context of strings/ string_views: bing.com/… \$\endgroup\$ – aki Aug 21 '20 at 14:06
  • \$\begingroup\$ Why adopt this style instead of early returns? Something like this: pastebin.com/997kVR4Q? The function stops looking so complex and is easier to understand and maintain. \$\endgroup\$ – Ismael Miguel Aug 21 '20 at 14:45
  • 1
    \$\begingroup\$ @IsmaelMiguel That's really a question for the OP and not for me. I kept their logic intact as much as I could. \$\endgroup\$ – pacmaninbw Aug 21 '20 at 15:03
  • 2
    \$\begingroup\$ @anki: Ok, "clearer" isn't the real reason. Being technically accurate without having to use the obscure term "NUL" is the reason. The fact that "null-terminated" is widely mis-used for strings (rather than pointer-based data structures like linked lists) doesn't mean you should, too. (Unlike natural language which does evolve new meanings and usage, technical language should be kept pure for maximum clarity.) \$\endgroup\$ – Peter Cordes Aug 21 '20 at 18:55
10
\$\begingroup\$
  • 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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ It is not a big effort to handle all of them That's an overstatement, unless you just mean naive best-effort without actually getting the nearest float for every decimal string input. Real-world atof implementations typically use big-integer extended precision to handle integer and fractional parts. e.g. the MUSL libc implementation git.musl-libc.org/cgit/musl/tree/src/internal/… is (according to the author) pretty dense but self-contained. \$\endgroup\$ – Peter Cordes Aug 21 '20 at 9:47
  • \$\begingroup\$ Eric Postpischil has an answer on an (unfortunately deleted) SO question with discussion and example code. stackoverflow.com/questions/51301315/… Needs some more votes to undelete the question. It points out the potential difficulties with integer overflow of the integer part, and that you potentially need all the decimal digits for rounding decisions. \$\endgroup\$ – Peter Cordes Aug 21 '20 at 9:52
  • \$\begingroup\$ Whole articles have been written on accurately implementing strtod, such as exploringbinary.com/how-strtod-works-and-sometimes-doesnt about David Gay's widely-used implementation, vs. GLIBC's pure-integer version exploringbinary.com/how-glibc-strtod-works \$\endgroup\$ – Peter Cordes Aug 21 '20 at 9:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.