Implementing atoi() in C

Question

I was given a school assignment to implement atoi(). Please review my code.

int my_atoi(char *ato)
{
    int res;
    int sign;

    res = 0;
    sign = 1;
    while (*ato == ' ' || *ato == '\t' || *ato == '\n' || *ato == '\f' || *ato == '\r')
        ato++;
    if (*ato == '-')
        sign = -1;
    if (*ato == '-' || *ato == '+')
         ato++;
    while (*ato >= '0' && *ato <= '9')
    {
        res = res * 10 + *ato - '0';
        ato++;
    }
    return (sign * res);
}

Answer 1

Be consistent with stdlib.h declaration of atoi()

The standard declaration is int atoi(const char *str);. Your declaration should match (i.e., take a const char * instead of char *).

What happens if a NULL pointer is passed?

As written, you dereference ato without checking if it is NULL (0). That is a big no-no in C. At the very top of the function, you should check if it's 0 and return 0 if it is:

if (!ato) { return 0; }

Edit: Martin R points out that silently returning 0 when a NULL pointer is passed to your function, as I have suggested, is bad programming practice, because it hides the error. If there's a possibility of calling myatoi() with a NULL pointer, there's no way for the function to notify the caller of the error. Therefore, it's incumbent upon the caller to check before calling.

No need for separate variable definition and initialization

Do:

int res = 0;
int sign = 0;

Don't:

int res;
int sign;
res = 0;
sign = 1;

Don't reinvent the wheel

Your first loop that skips over whitespace could use the standard library function isspace():

Edit: spot the bug in my original code... :/

Buggy (oops): while (isspace(*ato++));
Correct:      while (isspace(*ato)) ato++;

The condition check in the second loop can use the stdlib function isdigit():

while (isdigit(*ato)) {
    ...
    ato++;
}

If your instructor or assignment instructions prevent you from using isspace() and isdigit(), why not go ahead and define them yourself? You already have the logic!^*

^* Except you have a minor bug in your check for space characters. You also need to check for vertical tab (\v).

Edit: Chux points out that calling isspace(*ato) and isdigit(*ato) when *ato < 0 invokes undefined behavior (UB). Specifically, the C11 standard, section 7.4, Character Handling, states,

The header <ctype.h> declares several functions useful for classifying and mapping characters. In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

The correct usage, then, of the isspace() and isdigit() functions in this code would be:

while (isspace(*(unsigned char *)ato)) { atoi++; }
//...
while (isdigit(*(unsigned char *)ato)) {
    //...
    ato++;
}

Chux also pointed out that your original code as written did not have this problem; you weren't invoking undefined behavior. Moral of the story: be careful of the advice you receive from strangers on the Internet...? =)

---

Other than the issues mentioned, you have a decent implementation!

Answer 2

Overall, I think this is a really good straightforward implementation, which means it's easy to read and understand. Nice work! I only have a few minor things I'd suggest:

Don't Reinvent the Wheel (Even When Reinventing the Wheel!)

There's a function to tell if something is whitespace. It's called isspace(). Your first loop could be rewritten as:

while (isspace(*ato))
    ato++;

Use const where appropriate

The argument to your function shouldn't be changed (at least from the perspective of someone calling it). As such, it should be marked as const:

int my_atoi(const char* ato)

Note that this will mean you'll have to copy it and have an extra local variable. But the trade-off in readability and in the ability to reason about the function is worth it. Now when developers who are using your function see the prototype, they'll immediately know that the value is not changed by the function.

EDIT: As usual, I forgot my const rules. const type* means that the thing pointed to by the pointer is constant, not that the pointer itself is constant. So forget what I said above. You can just use const char* and not copy the pointer.

Naming

I would also clarify the names of variable. They're not terrible, but they could be better. Rather than res, why not at least result? Or better yet, something like numeric_value or something more descriptive? Likewise ato does not really tell me what it is. Maybe something like string_value or strVal at least. (In my opinion, the name of the library function is terrible, too. I didn't know it existed for the longest time because it's so poorly named.)

Other than that, this is pretty decent, in my opinion.

Answer 3

1. Avoidable integer overflow.
   When the text version for INT_MIN is passed, what happens? Both res * 10 + *ato - '0' and sign * res are experiencing int overflow, which is undefined behavior. This means, even if you get the hoped for result today, you may not get it tomorrow or on another complier/platform. Best to avoid. One method is to accumulate res amongst the negative side as there is the same or usually more negative numbers than positive one. (think +2147483647 vs. -2147483648).

   sign = *ato;  // simple save candidate sign character
   if (*ato == '-' || *ato == '+') {
     ato++;
   }
   while (isdigit(*ato)) {
     res = res * 10 - (*ato - '0');  // -
     ato++;
   }
   if (sign != '-') }
     res = -res;
   }
   return res;
   

2. Unavoidable out of range.
   atoi() has "If the value of the result cannot be represented, the behavior is undefined." Your my_atoi() could do better by preventing overflow. A simple method is to use a wider integer type. Of course that does not help if there is no wider type. If interested in code that prevents that without a wider type, let me know, as it is not technically needed, your function could have UB too if it is like atoi().

3. Proper use of is...()
   Recommend use of isspace(). The list of charcters used in OP's code lacks the standard '\v' and other spaces characters that are locale dependable, is...() functions are curious in that they are defined for unsigned char values and EOF. Avoid char ch; is....(ch); if ch may have a negative value. Similar idea for isdigit().

   // while (*ato == ' ' || *ato == '\t' || *ato == '\n' || *ato == '\f' || *ato == '\r')
   // while (isspace(*ato))   // UB the *ato < 0
   while (isspace((unsigned char) *ato))
   

Minor

4. No need to declare until the variable is needed.

   // int res;
   // res = 0;
   int res = 0; 
   

5. Note that this my_atoi(), like atoi(), does not define the result of my_atoi(""), my_atoi("xyz"), my_atoi("123xyz").

6. Note that handling my_atoi(NULL) is not needed, if like atoi(), as NULL is not a valid argument. It is popular to check for this though.