11

I was wondering if my implementation of an "itoa" function is correct. Maybe you can help me getting it a bit more "correct", I'm pretty sure I'm missing something. (Maybe there is already a library doing the conversion the way I want it to do, but... couldn't find any)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

char * itoa(int i) {
  char * res = malloc(8*sizeof(int));
  sprintf(res, "%d", i);
  return res;
}

int main(int argc, char *argv[]) {
 ...
idmean
  • 14,540
  • 9
  • 54
  • 83
Nicolas C.
  • 983
  • 1
  • 8
  • 12

14 Answers14

13
// Yet, another good itoa implementation
// returns: the length of the number string
int itoa(int value, char *sp, int radix)
{
    char tmp[16];// be careful with the length of the buffer
    char *tp = tmp;
    int i;
    unsigned v;

    int sign = (radix == 10 && value < 0);    
    if (sign)
        v = -value;
    else
        v = (unsigned)value;

    while (v || tp == tmp)
    {
        i = v % radix;
        v /= radix;
        if (i < 10)
          *tp++ = i+'0';
        else
          *tp++ = i + 'a' - 10;
    }

    int len = tp - tmp;

    if (sign) 
    {
        *sp++ = '-';
        len++;
    }

    while (tp > tmp)
        *sp++ = *--tp;

    return len;
}

// Usage Example:
char int_str[15]; // be careful with the length of the buffer
int n = 56789;
int len = itoa(n,int_str,10);
ZachB
  • 13,051
  • 4
  • 61
  • 89
Minh Nguyen
  • 139
  • 1
  • 2
7

The only actual error is that you don't check the return value of malloc for null.

The name itoa is kind of already taken for a function that's non-standard, but not that uncommon. It doesn't allocate memory, rather it writes to a buffer provided by the caller:

char *itoa(int value, char * str, int base);

If you don't want to rely on your platform having that, I would still advise following the pattern. String-handling functions which return newly allocated memory in C are generally more trouble than they're worth in the long run, because most of the time you end up doing further manipulation, and so you have to free lots of intermediate results. For example, compare:

void delete_temp_files() {
    char filename[20];
    strcpy(filename, "tmp_");
    char *endptr = filename + strlen(filename);
    for (int i = 0; i < 10; ++i) {
        itoa(endptr, i, 10); // itoa doesn't allocate memory
        unlink(filename);
    }
}

vs.

void delete_temp_files() {
    char filename[20];
    strcpy(filename, "tmp_");
    char *endptr = filename + strlen(filename);
    for (int i = 0; i < 10; ++i) {
        char *number = itoa(i, 10); // itoa allocates memory
        strcpy(endptr, number);
        free(number);
        unlink(filename);
    }
}

If you had reason to be especially concerned about performance (for instance if you're implementing a stdlib-style library including itoa), or if you were implementing bases that sprintf doesn't support, then you might consider not calling sprintf. But if you want a base 10 string, then your first instinct was right. There's absolutely nothing "incorrect" about the %d format specifier.

Here's a possible implementation of itoa, for base 10 only:

char *itobase10(char *buf, int value) {
    sprintf(buf, "%d", value);
    return buf;
}

Here's one which incorporates the snprintf-style approach to buffer lengths:

int itobase10n(char *buf, size_t sz, int value) {
    return snprintf(buf, sz, "%d", value);
}
Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
  • 1
    This is silly, if he's not returning allocated memory, then why not just call snprintf directly. `snprintf(endptr, bytes, "%d", number)` - This would prevent a temporary variable declaration and doesn't have those buffer overflow possibilities in this sample code. –  Aug 09 '10 at 17:12
  • @evilclown: the reason not to call `snprintf` directly the is the same reason anyone ever writes a function: to abstract the operation specifically of converting an integer to a string, as opposed to any of the many things which `snprintf` can do with different string formats. There is only a buffer overflow possibility in this code on platforms where `int` is at least 48 bits, and I think that issue has been covered well elsewhere. Feel free to merge one of those into this answer :-) – Steve Jessop Aug 09 '10 at 17:30
  • 1
    Anyway, snprintf is often over-rated. If you're worried about buffer overflows, and your solution is to use snprintf, then why aren't you worried about truncating the result string and getting the wrong answer? A "secure" program which doesn't actually work is of course better than an insecure program which doesn't work, but still not great ;-p – Steve Jessop Aug 09 '10 at 17:32
  • @Steve, that is why snprintf returns a negative when there is truncation, and you check for errors. Best of both worlds! –  Aug 09 '10 at 17:34
  • 1
    Standard `snprintf` doesn't return negative on truncation - that's a pre-C99-ism in e.g. Microsoft's `_snprintf` and very old glibc versions. It returns the number of bytes which would have been written, had the size been sufficiently large. Checking for negative return means you'll miss truncation on standard implementations. `snprintf` is good for the "measure, allocate, write" pattern, but it doesn't help much with short buffers IMO. And this is even before you worry about whether the length you've passed in is actually correct. Safe string handling in C is hard. – Steve Jessop Aug 09 '10 at 17:47
  • I didn't realize that was not standard, thanks for the info. I still would rather see snprintf than a function that simply wraps snprintf. I understand "abstracting" but I would not do it in this case. Would you recommend `strcpy_strings_that_start_with_a` instead of `strcpy`? –  Aug 09 '10 at 17:53
  • @evilclown: of course not, because the two would have exactly the same parameters and implementation. Would you recommend avoiding http libraries, in favour of manipulating sockets directly? We can play with misleading extremes indefinitely - in this case, the wrapper "removes" a parameter in order to create an "inverse" to the standard function `atoi`, so it doesn't do *much* compared with direct use of snprintf, but it does more than nothing. The existence of `atoi` makes `itoa` a particularly natural abstraction. – Steve Jessop Aug 09 '10 at 17:57
  • I guess ultimately I think it's always worth writing whatever functions match the abstractions that the *caller* wants (where possible, and performance permitting), even if they end up very similar to existing functions. If I found myself often wanting to copy strings which necessarily start with `a`, I guess maybe I would write a function which `asserts` the first character and then calls `strcpy`. On a release build, it would become a do-nothing wrapper. – Steve Jessop Aug 09 '10 at 18:05
6

A good int to string or itoa() has these properties;

  • Works for all [INT_MIN...INT_MAX], base [2...36] without buffer overflow.
  • Does not assume int size.
  • Does not require 2's complement.
  • Does not require unsigned to have a greater positive range than int. In other words, does not use unsigned.
  • Allows use of '-' for negative numbers, even when base != 10.

Tailor the error handling as needed. (needs C99 or later):

char* itostr(char *dest, size_t size, int a, int base) {
  // Max text needs occur with itostr(dest, size, INT_MIN, 2)
  char buffer[sizeof a * CHAR_BIT + 1 + 1]; 
  static const char digits[36] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";

  if (base < 2 || base > 36) {
    fprintf(stderr, "Invalid base");
    return NULL;
  }

  // Start filling from the end
  char* p = &buffer[sizeof buffer - 1];
  *p = '\0';

  // Work with negative `int`
  int an = a < 0 ? a : -a;  

  do {
    *(--p) = digits[-(an % base)];
    an /= base;
  } while (an);

  if (a < 0) {
    *(--p) = '-';
  }

  size_t size_used = &buffer[sizeof(buffer)] - p;
  if (size_used > size) {
    fprintf(stderr, "Scant buffer %zu > %zu", size_used , size);
    return NULL;
  }
  return memcpy(dest, p, size_used);
}
chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackoverflow.com/rooms/251311/discussion-on-answer-by-chux-reinstate-monica-what-is-the-proper-way-of-implem). – sideshowbarker Jan 22 '23 at 05:29
3

I think you are allocating perhaps too much memory. malloc(8*sizeof(int)) will give you 32 bytes on most machines, which is probably excessive for a text representation of an int.

kbrimington
  • 25,142
  • 5
  • 62
  • 74
  • You're right. Since 2^15 - 1 is actually 32767, I just need 6*sizeof(char) – Nicolas C. Aug 09 '10 at 14:14
  • 1
    @Nicolas: And one more for the sign, if considering negative numbers. – kbrimington Aug 09 '10 at 14:16
  • thanks ! (cannot +1, I'm not registered yet) but I would have ! – Nicolas C. Aug 09 '10 at 14:18
  • 6
    `sizeof(int)*3+2` should always be sufficient (each byte contributes 3 digits or fewer, and the extra 2 are for sign and null termination. – R.. GitHub STOP HELPING ICE Aug 09 '10 at 16:03
  • 1
    @R.: Not on architectures where `CHAR_BIT` is larger than 9. Eg. some DSPs have `CHAR_BIT == 32` and `sizeof(int) == 1`. – caf Aug 09 '10 at 23:51
  • @caf: Huh. How's that for something. One thing's for sure, SO helps to challenge my preconceptions. Thanks! – kbrimington Aug 10 '10 at 00:15
  • If you want to take account of that, you can instead use `((CHAR_BIT * sizeof(type) - 1) / 3 + 2)` for signed types, or `((CHAR_BIT * sizeof(type)) / 3 + 1)` for unsigned types. – caf Aug 10 '10 at 00:27
  • @caf: Nice! Thanks for the additional information! Being a .NET guy, I am impressed at how much their is to learn when you dip below the .NET/Java/Python/etc abstraction. – kbrimington Aug 10 '10 at 00:37
  • that's exactly why I asked this question; even with a question as simple as it was, I couldn't think of all the cases, and my programming style is not brilliant yet. – Nicolas C. Aug 10 '10 at 06:53
3

i found an interesting resource dealing with several different issues with the itoa implementation
you might wanna look it up too
itoa() implementations with performance tests

Adam
  • 171
  • 3
2

I'm not quite sure where you get 8*sizeof(int) as the maximum possible number of characters -- ceil(8 / (log(10) / log(2))) yields a multiplier of 3*. Additionally, under C99 and some older POSIX platforms you can create an accurately-allocating version with sprintf():

char *
itoa(int i) 
{
    int n = snprintf(NULL, 0, "%d", i) + 1;
    char *s = malloc(n);

    if (s != NULL)
        snprintf(s, n, "%d", i);
    return s;
}

HTH

llasram
  • 4,417
  • 28
  • 28
  • Two calls to snprintf seems like bad practice. –  Aug 09 '10 at 14:27
  • 1
    @evilclown: well, when the format string is just `%d` it's not really necessary, since you can work out the max required space at compile-time. For a 32bit int, it's 12, so you wouldn't usually worry too much about over-allocation. This general pattern of calling `snprintf` twice is absolutely standard stuff, though. – Steve Jessop Aug 09 '10 at 16:28
1

You should use a function in the printf family for this purpose. If you'll be writing the result to stdout or a file, use printf/fprintf. Otherwise, use snprintf with a buffer big enough to hold 3*sizeof(type)+2 bytes or more.

R.. GitHub STOP HELPING ICE
  • 208,859
  • 35
  • 376
  • 711
1

sprintf is quite slow, if performance matters it is probably not the best solution.

if the base argument is a power of 2 the conversion can be done with a shift and masking, and one can avoid reversing the string by recording the digits from the highest positions. For instance, something like this for base=16

int  num_iter = sizeof(int) / 4;

const char digits[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'};

/* skip zeros in the highest positions */
int i = num_iter;
for (; i >= 0; i--)
{
    int digit = (value >> (bits_per_digit*i)) & 15;
    if ( digit > 0 )  break;
}

for (; i >= 0; i--)
{
    int digit = (value >> (bits_per_digit*i)) & 15;
    result[len++] = digits[digit];
}

For decimals there is a nice idea to use a static array big enough to record the numbers in the reversed order, see here

1
  • Integer-to-ASCII needs to convert data from a standard integer type into an ASCII string.
  • All operations need to be performed using pointer arithmetic, not array indexing.
  • The number you wish to convert is passed in as a signed 32-bit integer.
  • You should be able to support bases 2 to 16 by specifying the integer value of the base you wish to convert to (base).
  • Copy the converted character string to the uint8_t* pointer passed in as a parameter (ptr).
  • The signed 32-bit number will have a maximum string size (Hint: Think base 2).
  • You must place a null terminator at the end of the converted c-string Function should return the length of the converted data (including a negative sign).
  • Example my_itoa(ptr, 1234, 10) should return an ASCII string length of 5 (including the null terminator).
  • This function needs to handle signed data.
  • You may not use any string functions or libraries.

.

uint8_t my_itoa(int32_t data, uint8_t *ptr, uint32_t base){
        uint8_t cnt=0,sgnd=0;
        uint8_t *tmp=calloc(32,sizeof(*tmp));
        if(!tmp){exit(1);}
        else{
            for(int i=0;i<32;i++){
            if(data<0){data=-data;sgnd=1;}
            if(data!=0){
               if(data%base<10){
                *(tmp+i)=(data%base)+48;
                data/=base;
               }
               else{
                *(tmp+i)=(data%base)+55;
                data/=base;
               }
            cnt++;     
            }
           }
        if(sgnd){*(tmp+cnt)=45;++cnt;}
        }
     my_reverse(tmp, cnt);
     my_memcopy(tmp,ptr,cnt);
     return ++cnt;
}
  • ASCII-to-Integer needs to convert data back from an ASCII represented string into an integer type.
  • All operations need to be performed using pointer arithmetic, not array indexing
  • The character string to convert is passed in as a uint8_t * pointer (ptr).
  • The number of digits in your character set is passed in as a uint8_t integer (digits).
  • You should be able to support bases 2 to 16.
  • The converted 32-bit signed integer should be returned.
  • This function needs to handle signed data.
  • You may not use any string functions or libraries.

.

int32_t my_atoi(uint8_t *ptr, uint8_t digits, uint32_t base){
    int32_t sgnd=0, rslt=0;
    for(int i=0; i<digits; i++){
        if(*(ptr)=='-'){*ptr='0';sgnd=1;}
        else if(*(ptr+i)>'9'){rslt+=(*(ptr+i)-'7');}
        else{rslt+=(*(ptr+i)-'0');}
        if(!*(ptr+i+1)){break;}
        rslt*=base;
    }
    if(sgnd){rslt=-rslt;}
    return rslt;
}
1

I don't know about good, but this is my implementation that I did while learning C

static int  ft_getintlen(int value)
{
    int l;
    int neg;

    l = 1;
    neg = 1;
    if (value < 0)
    {
        value *= -1;
        neg = -1;
    }
    while (value > 9)
    {
        l++;
        value /= 10;
    }
    if (neg == -1)
    {
        return (l + 1);
    }
    return (l);
}

static int  ft_isneg(int n)
{
    if (n < 0)
        return (-1);
    return (1);
}

static char *ft_strcpy(char *dest, const char *src)
{
    unsigned int    i;

    i = 0;
    while (src[i] != '\0')
    {
        dest[i] = src[i];
        i++;
    }
    dest[i] = src[i];
    return (dest);
}

char    *ft_itoa(int n)
{
    size_t  len;
    char    *instr;
    int     neg;

    neg = ft_isneg(n);
    len = ft_getintlen(n);
    instr = (char *)malloc((sizeof(char) * len) + 1);
    if (n == -2147483648)
        return (ft_strcpy(instr, "-2147483648"));
    if (!instr)
        return (NULL);
    if (neg == -1)
        n *= -1;
    instr[len--] = 0;
    if (n == 0)
        instr[len--] = 48;
    while (n)
    {
        instr[len--] = ((n % 10) + 48);
        n /= 10;
    }
    if (neg == -1)
        instr[len] = '-';
    return (instr);
}

Anarb
  • 11
  • 1
0

There a couple of suggestions I might make. You can use a static buffer and strdup to avoid repeatedly allocating too much memory on subsequent calls. I would also add some error checking.

char *itoa(int i)
{
  static char buffer[12];

  if (snprintf(buffer, sizeof(buffer), "%d", i) < 0)
    return NULL;

  return strdup(buffer);
}

If this will be called in a multithreaded environment, remove "static" from the buffer declaration.

Brandon Horsley
  • 7,956
  • 1
  • 29
  • 28
0

This should work:

#include <string.h>
#include <stdlib.h>
#include <math.h>

char * itoa_alloc(int x) {
   int s = x<=0 ? 1 ? 0; // either space for a - or for a 0
   size_t len = (size_t) ceil( log10( abs(x) ) );
   char * str = malloc(len+s + 1);

   sprintf(str, "%i", x);

   return str;
}

If you don't want to have to use the math/floating point functions (and have to link in the math libraries) you should be able to find non-floating point versions of log10 by searching the Web and do:

size_t len = my_log10( abs(x) ) + 1;

That might give you 1 more byte than you needed, but you'd have enough.

nategoose
  • 12,054
  • 27
  • 42
0

This is chux's code without safety checks and the ifs. Try it online:

char* itostr(char * const dest, size_t const sz, int a, int const base) {
  bool posa = a >= 0;

  char buffer[sizeof a * CHAR_BIT + 1]; 
  static const char digits[36] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";

  char* p = &buffer[sizeof buffer - 1];
  
  do {
    *(p--) = digits[abs(a % base)];
    a /= base;
  } while (a);

  *p = '-';
  p += posa;

  size_t s = &buffer[sizeof(buffer)] - p;
  memcpy(dest, p, s);
  dest[s] = '\0';

  return dest;
}
user1095108
  • 14,119
  • 9
  • 58
  • 116
-1
main()
{
  int i=1234;
  char stmp[10];
#if _MSC_VER
  puts(_itoa(i,stmp,10));
#else
  puts((sprintf(stmp,"%d",i),stmp));
#endif
  return 0;
}
user411313
  • 3,930
  • 19
  • 16