7
\$\begingroup\$

I'm looking for feedback on my atoi implementation here in C. Any feedback would great. I made my own atoi for fun and to learn about it.

I'm not trying to replace the standard library version. Reinventing the wheel is fun to me.

#include <limits.h> /* for LONG_MAX, INT_MAX */
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

#define IS_ASCII_DIGIT(c) ((c >= 48) && (c <= 57))

long __my_atoi(char* buffer)
{
    long ret = 0;
    bool neg = false;

    if (*buffer == '-') {
        neg = true;
        buffer++; /* advance to next position to pass ascii check */
    }

    while (*buffer) {
        if (IS_ASCII_DIGIT(*buffer)) {
            ret = ret * 10 + (*buffer - '0');
        } else {
            fprintf(stderr, "Fatal Error: unexpected '%c' passed to %s\n", *buffer, __func__);
            exit(EXIT_FAILURE);
        }
        buffer++;
    }

    return neg ? -ret : ret;
}

int main(void)
{
    printf("%ld\n", __my_atoi("-10004"));
    return EXIT_SUCCESS;
}
\$\endgroup\$
7
\$\begingroup\$
  • If implementing your own version of a standard routine such as atoi(), some form of explanation (comment) is needed. For example, is this supposed to be a drop in replacement for atoi()? Are some aspects deliberately not implemented? Do you have space restrictions that prevent you from using the standard? Do you know that certain string combinations will not exist? This type of information is helpful to anyone reading your code so that they can put things into context.

For the remainder of this review, I shall be assuming that it is meant as an exact replacement for atoi().

  • The header file limits.h is included, but I do not see any current reason for it to be there.

  • IS_ASCII_DIGIT() should not be using the magic numbers of 48 and 57. Values such as '0' and '9' are preferred.

  • It is customary to surround the macro arguments with parentheses. This helps to ensure that the argument is evaluated as a whole to cover the case where the argument is a complex expression.
  • Instead of using IS_ASCII_DIGIT(), I would recommend using isdigit(). This would require including the header file ctype.h.

  • atoi() is supposed to return an int. Your routine returns a long. This indicates either a misnamed routine or an incorrect return value.

  • The contents of buffer parameter do not change. It would be better to declare it as a 'const char *'.
  • There is nothing preventing the dereferencing a NULL pointer.

  • There is no accounting for white space at the beginning of the string.

  • There is no accounting for the '+' character at the start of the integer sequence.
  • There is no accounting for values that are too large or too small to represent.

  • The standard atoi() routine does not exit or print any message upon a detected error; neither should yours.

For additional information on atoi(), please refer to the following pages:

Hope this helps.

\$\endgroup\$
  • \$\begingroup\$ See my edit. I made my version for fun. I'm not trying to replace standard library version. Can you appreciate that? \$\endgroup\$ – Ryan M Mar 21 '15 at 13:57
  • 1
    \$\begingroup\$ I can indeed appreciate it. It can be a very educational process that can lead to many insights. I do hear you that you made your version for fun. I want you to know that I was not (and am not) trying to be harsh--only thorough. Also, please keep in mind that only standard I had to judge it against for its apparent correctness was the behaviour of the standard implementation (as described at the links). \$\endgroup\$ – Sparky Mar 21 '15 at 14:47
  • \$\begingroup\$ I can appreciate your thorough feedback :) \$\endgroup\$ – Ryan M Mar 21 '15 at 15:05
  • \$\begingroup\$ OP's IS_ASCII_DIGIT(*buffer) is better than isdigit(*buffer) as isdigit(*buffer) is UB when *buffer < 0 and OP's is well defined. \$\endgroup\$ – chux - Reinstate Monica Feb 8 at 10:30
  • \$\begingroup\$ 48 is* better to use than '0' for ASCII conversion testing, yet STL atoi() is not ASCII specific - even though ASCII is ubiquitous. '0' is better for a correct atoi(). \$\endgroup\$ – chux - Reinstate Monica Feb 8 at 10:34
4
\$\begingroup\$

For one thing you might want to be a bit more forgiving with non-digits. The original atoi just returns 0 if there are non-digits, terminating the program due to a non-digit is a bit harsh.


underscores in front of your function name is not recommended, my_atoi would do.


check the parameter if it is null before dereferencing it to avoid error, just return 0 - optionally print a message to stderr.


other than that i find it was quite clever way of calculating atoi, it took me a moment to understand it -- sometimes it is better to let the compiler optimize the code for you than try to write too clever code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Important side note: Names beginning with double underscores are reserved for the implementation, so __my_atoi is actually undefined behavior. \$\endgroup\$ – Ben Steffan Dec 23 '17 at 19:55
  • \$\begingroup\$ according to the C standard: "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use" not sure if it is undefined behavoir but it could cause unnecessary issues. \$\endgroup\$ – Anders Dec 23 '17 at 20:55
  • \$\begingroup\$ If the program declares or defines an identifier in a context in which it is reserved (other than as allowed by 7.1.4), or defines a reserved identifier as a macro name, the behavior is undefined. Since every identifier beginning with two underscores is reserved in every context, this implies UB (if I am reading this correctly). \$\endgroup\$ – Ben Steffan Dec 23 '17 at 21:11
  • \$\begingroup\$ either way we can agree it is not a good thing :) merry xmas! \$\endgroup\$ – Anders Dec 24 '17 at 6:18
4
\$\begingroup\$

IS_ASCII_DIGIT may fail for non-ascii locales. isdigit is much more reliable. If you want to use it anyway, replace magic numbers 48 and 57 with '0' and '9' respectively. It is also highly recommended to parenthesise the macro argument, as in

    (((c) >= 48) && ((c) <= 57))

It doesn't really matter here, but may spare you from unpleasant surprises like in

    IS_ASCII_DIGIT(a + b)
\$\endgroup\$
  • \$\begingroup\$ Detail: IS_ASCII_DIGIT would not "fail" to run for rare non-ASCII locales. It is that in non-ASCII locales, OP's code would differ than atoi() and fail to match functionality. Yet it would then function "correctly" is the coding goal was atoi_ASCII() even in non-ASCII locales. \$\endgroup\$ – chux - Reinstate Monica Feb 8 at 10:44
  • \$\begingroup\$ isdigit is not much more reliable as it is often mis-coded - this answer does not posted its use, so the advice is only half way there. isdigit(*buffer) is UB when *buffer < 0. isdigit((unsigned char) *buffer) is better and matches atoi specification. \$\endgroup\$ – chux - Reinstate Monica Feb 8 at 10:48
1
\$\begingroup\$

Unnecessary undefined behavior

ret = ret * 10 + (*buffer - '0'); incurs signed integer overflow and undefined behavior (UB) when the final return value is expected to be LONG_MIN.

Various alternatives:

Best:
Use ret = ret * 10 - (*buffer - '0'); and then at the end negate when the sign was not '-'. This approach is a bit off-putting to read, yet is very well specified.

Good:
Use unsigned long ret as the unsigned complement to long can accommodate -LONG_MIN. This works for all but the most esoteric platforms.

Fair:
Live with UB. The UB is usually OK here.

Detect digits by local encoding

atoi() works with the compiler encoding of '0' which is not specified to be ASCII. Code can continue to use something IS_ASCII_DIGIT(c) if the goal is to convert input as if it was ASCII even if on a rare non-ASCII platform. To be closer to atoi(), test as follows.

// #define IS_ASCII_DIGIT(c) ((c >= 48) && (c <= 57))
#define IS_DIGIT(c) (isdigit((unsigned char) (c))
// or 
#define IS_DIGIT(c) (((c) >= '0') && ((c) <= '9'))
\$\endgroup\$

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.