tags:

views:

357

answers:

8

First, hi, I'm new to stackoverflow !

Well, 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[]) {
 ...

Thank you.

+3  A: 

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
You're right. Since 2^15 - 1 is actually 32767, I just need 6*sizeof(char)
Nicolas C.
@Nicolas: And one more for the sign, if considering negative numbers.
kbrimington
thanks ! (cannot +1, I'm not registered yet) but I would have !
Nicolas C.
`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..
@R.: Not on architectures where `CHAR_BIT` is larger than 9. Eg. some DSPs have `CHAR_BIT == 32` and `sizeof(int) == 1`.
caf
@caf: Huh. How's that for something. One thing's for sure, SO helps to challenge my preconceptions. Thanks!
kbrimington
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
@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
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.
A: 
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;
}
+2  A: 

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
A: 

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
Two calls to snprintf seems like bad practice.
@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
+1  A: 

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
Seems like a lot of work to calculate the right size when the maximum is a known 12 bytes. ceil, log10, abs, two temporary variables (totalling 8 bytes) and a ternary operation.
@evilclown: Yeah, but the general process works for any sized integer.
nategoose
+1  A: 

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..
+2  A: 

The only actual error is that you don't check the return value of malloc for null. And on some systems, that isn't even an error.

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
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.
@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
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
@Steve, that is why snprintf returns a negative when there is truncation, and you check for errors. Best of both worlds!
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
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`?
@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
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
A: 

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