char* to integer implementation (atoi with saturation to INT_MIN or INT_MAX on overflow)

Question

I am learning C. As a task, I need to write a get_int function that will convert a char* to int. What parts can I improve here? I'm especially interested in how I can improve/rewrite the overflow checking.

#include <stdio.h>
#include <ctype.h>
#include <limits.h>

#define MINIUNIT_MAIN
// https://github.com/urin/miniunit/blob/master/miniunit.h
#include "miniunit.h"

int get_int(char *string) {
  int i = 0;

  while (isspace(string[i])) {
    i++;
  }

  char sign = string[i] == '-' ? '-' : '+';
  if (string[i] == '-' || string[i] == '+') {
    i++;
  }

  int result = 0;

  while (string[i] != '\0' && isdigit(string[i])) {
    // Check for overflow
    if (INT_MIN / 10 > result || (INT_MIN / 10 == result && INT_MIN % 10 >= -(string[i] - '0'))) {
      return sign == '-' ? INT_MIN : INT_MAX;
    }

    result = result * 10 - (string[i] - '0');

    i++;
  }

  return sign == '-' ? result : -result;
}

int main(void) {
  test_case("Simple numbers", {
    expect("0", get_int("0") == 0);

    expect("1", get_int("1") == 1);

    expect("12", get_int("12") == 12);

    expect("199", get_int("199") == 199);

    expect("1000", get_int("1000") == 1000);

    expect("214748364", get_int("214748364") == 214748364);

    expect("214748500", get_int("214748500") == 214748500);
  });

  test_case("Invalid input", {
    expect("Empty string", get_int("") == 0);

    expect("Spaces", get_int("   ") == 0);

    expect("Spaces and numbers", get_int("   123") == 123);

    expect("Plus sign", get_int("+") == 0);

    expect("Minus sign", get_int("-") == 0);

    expect("Letters", get_int("abc") == 0);

    expect("Letters and numbers", get_int("123abc") == 123);

    expect("Dot and numbers", get_int("123.4") == 123);
  });

  test_case("Negative numbers", {
    expect("-0", get_int("-0") == 0);

    expect("-1", get_int("-1") == -1);

    expect("-5", get_int("-5") == -5);

    expect("-12", get_int("-12") == -12);

    expect("-1000", get_int("-1000") == -1000);
  });

  test_case("Overflows", {
    expect("INT_MAX", get_int("2147483647") == INT_MAX);

    expect("INT_MAX + 1", get_int("2147483648") == INT_MAX);

    expect("INT_MAX + 10", get_int("2147483657") == INT_MAX);

    expect("INT_MAX + 1000", get_int("2147484647") == INT_MAX);

    expect("INT_MIN", get_int("-2147483648") == INT_MIN);

    expect("INT_MIN - 1", get_int("-2147483649") == INT_MIN);

    expect("INT_MIN - 10", get_int("-2147483658") == INT_MIN);

    expect("INT_MIN - 1000", get_int("-2147484648") == INT_MIN);
  });
}

Answer 1

Uncommon bug: is...()

is...() expects to receive an int with a value in the unsigned char range or EOF. char values may be other negatives which results in undefined behavior (UB).

//  while (isspace(string[i])) {
while (isspace(((unsigned char *)string)[i])) {

Pedantic bug: Long strings

int i = 0; better as size_t i = 0 for strings whose length exceeds INT_MAX as in pathological input "000...3 billion zeros... 123".

Overflow checking correct.

Test is good and works for int of various width and encodings.

Consider a stronger && test. It may help performance as first test INT_MIN / 10 > result is commonly false.

// if (INT_MIN / 10 > result || (INT_MIN / 10 == result && INT_MIN % 10 >= -(string[i] - '0'))) {
   if (INT_MIN / 10 >= result && (INT_MIN / 10 > result || INT_MIN % 10 >= -(string[i] - '0'))) {

Misleading comment

// Check for overflow also is the return path for valid input like get_int("-2147483648").

Name

get_int() sounds incorrect as get...() is often an I/O function. Maybe my_atoi().

Consider assigning errno so caller can distinguish good from bad.

On overflow and no conversion consider setting errno.

Maybe also invalid arguments and/or trailing non-numeric non-white-space text.

---

Sample tested alternative

• Use const.

• Stronger first range test. &&

• Add error indication when possible.

• Take advantage that isdigt('\0') is false.

• Stronger tests for argument validity.

#include <errno.h>
#include <stdlib.h>
#include <stdbool.h>

int atoint_alt(const char *string) {
  // Maybe check argument?
  if (string == NULL) {
#ifdef EINVAL
    errno = EINVAL;
#endif
    return 0;
  }
  
  bool digit_found = false;
  const unsigned char *ustring = (const unsigned char*) string;

  while (isspace(*ustring)) {
    ustring++;
  }

  bool isneg = *ustring == '-';
  if (isneg || *ustring == '+') {
    ustring++;
  }

  int result = 0;

  while (isdigit(*ustring)) {
    int digit = *ustring - '0';
    digit_found = true;
    // Check for overflow
    if (result <= INT_MIN / 10
        && (result < INT_MIN / 10 || digit > -(INT_MIN % 10))) {
      errno = ERANGE;
      return isneg ? INT_MIN : INT_MAX;
    }
    // Accumulate the value on the negative side.
    result = result * 10 - digit;
    ustring++;
  }

  if (!digit_found) {
#ifdef EINVAL
    errno = EINVAL;
#endif
    return 0;
  }

  if (!isneg) {
    if (result < -INT_MAX) {
      errno = ERANGE;
      return INT_MAX;
    }
    result = -result;
  }

  // Maybe look for extra junk
  while (isspace(*ustring)) {
    ustring++;
  }
  if (*ustring) {
#ifdef EINVAL
    errno = EINVAL;
#endif
    return 0;  // or maybe retval?
  }

  return result;
}

Note: INT_MIN / 10 and INT_MIN % 10 have some issues with pre-C99 code. Look to div() to fix.

Answer 2

The code is not bad and easy to follow. Here are some suggestions:

Use pointers rather than indexing

The i variable isn't really needed. Instead, you could simply increment the pointer passed into your get_int function.

Simplify variable usage

The sign variable is declared as a char and holds the sign, but every time it's used, it's compared with '-'. I'd suggest comparing once and declaring it bool like this:

bool negative = *string == '-'; 

This simplifies usage as with the return statement:

return negative ? -result : result;

Simplify overflow checking

This was rewritten thanks to insightful comments. As was pointed out in the comments to the older version of this answer, you need to catch overflow before it happens, since if it happens, it's undefined behavior (UB). This breaks the check into two pieces -- one for the multiplication and one for the addition.

for ( ; *string != '\0' && isdigit(*string); ++string) {
  if (result > INT_MAX/10) {
    return negative ? INT_MIN : INT_MAX;
  }
  result *= 10;
  if (result > INT_MAX - (*string - '0')) {
    return negative ? INT_MIN : INT_MAX;
  }
  result += (*string - '0');
}

Consider an explicit error return

Right now, it's not possible to tell the difference between an actual conversion resulting in 0 or if it's an error. Even more potentially confusing for users is that INT_MIN or INT_MAX might be returned either as an actual calculated value or as an error indicator. There are many ways to handle this: one way, used by the library function strtof, is to set the global errno value, but there are other ways as well.

Use const when passing parameters

Since your function does not and should not modify the passed string, you can declare it const to let the caller know that is the case.

Putting it all together

Applying all of these, here's the version I came up with:

int get_int(const char *string) {
  for ( ; isspace(*string); ++string) {
  }
  bool negative = *string == '-'; 
  if (*string == '-' || *string == '+') {
    ++string;
  }
  int result = 0;
  for ( ; *string != '\0' && isdigit(*string); ++string) {
    if (result > INT_MAX/10) {
      return negative ? INT_MIN : INT_MAX;
    }
    result *= 10;
    if (result > INT_MAX - (*string - '0')) {
      return negative ? INT_MIN : INT_MAX;
    }
    result += (*string - '0');
  }
  return negative ? -result : result;
}

Answer 3

Not an answer but a sample test harness using standard library code.

Adjust to ignore errno compare.

int strtoi(const char *s, char **endptr, int base) {
  if (s == NULL) {
    return 0;
  }
  long y = strtol(s, endptr, base);
  if (y < INT_MIN) {
    y = INT_MIN;
    errno = ERANGE;
  } else if (y > INT_MAX) {
    y = INT_MAX;
    errno = ERANGE;
  }
  return (int) y;
}

int main() {
  const char *s[] = { //
          // Must match value and errno
          "-2147483649", "-2147483648", "-2147483647", "-10", "-1", "-0", "0",
          "+0", "1", "10", "2147483646", "2147483647", "2147483648", "+1",
          "+10", "+2147483646", "+2147483647", "+2147483648", " \t\v\n\r123",
          " 42\n",  //
          // Must match value
          "x", "6000000000", "    ", "", "+", "-" "--123", "123.4", "abc",
          "876-5304", NULL};
  size_t n = sizeof s / sizeof s[0];
  bool must_match = true; // Must match both vlaue and errno.
  int failure_count = 0;
  for (size_t i = 0; i < n; i++) {
    errno = 0;
    int y0 = strtoi(s[i], 0, 10);
    int e0 = errno;
    errno = 0;
    int y1 = get_int0(s[i]);
    // int y1 = get_int_OP(s[i]);
    // int y1 = atoint_alt(s[i]);
    int e1 = errno;
    if (s[i] && s[i][0] == 'x') {
      must_match = false; // Only need match value.
    }
    if (y0 != y1 || (e0 != e1)) {
      bool fail = y0 != y1 || (must_match ? e0 != e1 : 0);
      failure_count += fail;
      printf("%d %9s ", failure_count, fail ? "fail" : "optional");
      printf("y0:%11d,%2d  y1:%11d,%2d <%s>\n", y0, e0, y1, e1,
          s[i] ? s[i] : "NULL");
    }
  }
  printf("Failure count %d\n", failure_count);
}

Sample output

0  optional y0:          0, 0  y1:          0,22 <x>
0  optional y0:          0, 0  y1:          0,22 <    >
0  optional y0:          0, 0  y1:          0,22 <>
0  optional y0:          0, 0  y1:          0,22 <+>
0  optional y0:          0, 0  y1:          0,22 <---123>
0  optional y0:        123, 0  y1:        123,22 <123.4>
0  optional y0:          0, 0  y1:          0,22 <abc>
0  optional y0:        876, 0  y1:        876,22 <876-5304>
0  optional y0:          0, 0  y1:          0,22 <NULL>
Failure count 0