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

• Toby Speight: You should probably show us your test suite too - the overflow bugs in this code suggest that you are missing the tests for very large (positive or negative) values.

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!

• user14393: It is convenient to be able to set a flag to compile a debug implementation where you do check for null pointer and emit a diagnostic (and abort)
• scottbb: @Hurkyl of course, what you're referring to is the use of assert() macro. When you no longer want the heavy debugging instrumentation that assert() typically carries along with it, you define NDEBUG, and the assert() typically gets defined out with something like, #define assert(ignore)((void) 0). Note that NDEBUG is not specified by C or C++ standards (although most compilers provide for it in their standard library).
• user14393: Actually, the C standard does specify that NDEBUG is used in that fashion (e.g. in a c11 draft, the section titled 7.2 Diagnostics <assert.h>)
• chux: isspace(*ato) is UB when *ato < 0. Op's white-space (though incomplete) is not UB. Same for isdigit().
• CiaPan: @scottbb Such abstraction is very useful, especially when we need portability. There is no problem with digits, as far as I know, recall, however, there are encodings (e.g. EBCDIC) where alphabet does not make a continuous block. In such encodings 'a' <= c && c <= 'z' is not necessarily equivalent to islower(c)...

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.

• scottbb: Crap, didn't notice you beat me to the punch! =)
• user1118321: No worries! You definitely caught some things I didn't. :-)
• JS1: I don't think that making the argument const would require copying it to a local variable. You can still move the pointer. You just can't write to it.
• user1118321: Perhaps you're right. I always forget that const when used with pointers as above means the data pointed to is constant, not that the pointer is. I'll amend my answer.
• Ruslan: If you wanted to make ato itself const, it'd be const char* const instead — or even char* const if you wanted to change the pointee but not the pointer.

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 [phone] 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.

• smac89: Should be res * -10? To make the entire value negative
• chux: @Smac89 No - still should be res * 10 - (*ato - '0'). It is still base 10, not base -10. res at that point is negative (or zero).
• CiaPan: If one could use isdigit from a standard library, why not use the standard atoi and do int my_atoi(const char *s) { return atoi(s); } ...?
• chux: @CiaPan Interesting point, or use (int)strtol(nptr, (char **)NULL, 10)? IAC, it is an exercise and the typical implications are that such trivial substitutions are not allowed. But to your point, code should then call/implement my_isspace().
• CiaPan: @chux Yes – and that's what OP, julekgwa, tried to do, performing explicit comparision to a space, tab, newline.. characters.