K&R atoi-general memory leak

Question

I am following K&R second edition examples to learn C and coding as I think this is correct way of doing things. Anyhow when I run this program post compilation the program get stuck. I used valgrind for execution of compiled script.

#include <ctype.h>

int atoi(char s[])
{
    int i, n, sign;
    for(i = 0; isspace(s[i]); i++)
        ;
    sign = (s[i] == '-')? -1 : 1;
    if (s[i] == '+'|| s[i] == '-')
        i++;
    for (int n = 0; isdigit(s[i]); n++)
        n = 10 * n + (s[i]-'0');
    return sign * n;
}

int main()
{
    char input[] = "       -12345";
    atoi(input);
}
# vaibhavchauhan at startup001 in ~/Documents/Projects/K-R on git:master x [0:43:36]
$ valgrind ./atoi-general
==8075== Memcheck, a memory error detector
==8075== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==8075== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==8075== Command: ./atoi-general
==8075==
^C==8075==

Answer 1

In your second loop, you are iterating n but you use i for your computations and computations. This leads to the infinite loop you observe. To fix this, use either i as an index consistently:

int atoi(char s[])
{
    int i, sign, n;
    for(i = 0; isspace(s[i]); i++)
        ;
    sign = (s[i] == '-')? -1 : 1;
    if (s[i] == '+'|| s[i] == '-')
        i++;
    for (n = 0; isdigit(s[i]); i++)
        n = 10 * n + (s[i]-'0');
    return sign * n;
}

Note that indices should have type size_t, not int as the latter might be not large enough to index every array. For this purpose, an index of type int is fine though.

Answer 2

As noticed, the index in your second loop is incorrect.

for(int n = 0; isdigit(s[i]); n++)
    n = 10 * n + (s[i]-'0');

This should be - notice that you should NOT reinitialize the index i in the second loop as it should be the left over value from the first loop (as you already skipped the white spaces and the sign).

for( ; isdigit(s[i]); i++ )
    n = 10 * n + (s[i]-'0');

Answer 3

The K&R example could be improved for readability and performance. The following issues exist:

• You are not allowed to name your own functions the same as standard lib functions. atoi exists in stdlib.h.
• Use const correctness for the passed parameter.
• The multiple loops and iterators are hard to read and maintain, which caused the bug in the question. Ideally there should only be one iterator. Arguably, this bug was caused by the original programmer(s), since they wrote unmaintainable, sloppy code.
• The type size_t should be used for the iterators, not int.
• It doesn't hurt if that iterator has a meaningful name either. n is a very bad name for an iterator! In C programming, n has a special meaning and that is to describe the number of items in a container.
• For the sake of readability and possibly also for performance, the pointer parameter can be used as iterator. Then we don't need to invent all these local iterator variables in the first place.
• Use a bool variable for the sign to increase readability.
• There is no need to check if the current character is - twice.
• Always use braces after every statement, to prevent fatal bugs, like the Apple goto fail.

---

#include <ctype.h>
#include <stdbool.h>


int my_atoi (const char* str)
{
  while(isspace(*str))
  {
    str++;
  }

  bool sign = false;
  if(*str == '-')
  {
    sign = true;
    str++;
  }
  else if(*str == '+')
  {
    str++;
  }

  int result = 0;
  while(isdigit(*str))
  {
    result = 10*result + *str - '0';
    str++;
  }

  if(sign)
  {
    result = -result;
  } 

  return result;
}