CODEWITHSUNDEEP
carraysstringpointers
ASlackjaw
ASlackjaw

Reputation: 11

Array entries being overwriten in C

I want to convert an integer array to a string array. For example, if arr[] = {1, 2, 3, 4, 5}, I want to end up with arr2[] = {"1", "2", "3", "4", "5"}. This function works fine until it exits the for loop where all the array entries are being overwritten with the value of the last entry. Any idea as to why this might be happening?

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

#define SIZE 5
int main(){
    int nums[] = {1, 2, 3, 4, 5};
    char **strings = (char **)malloc(SIZE * sizeof(char*));

    for(int i = 0; i < SIZE; i++){
        char temp[20];
        sprintf(temp, "%d", nums[i]);
        strings[i] = temp;
        //prints correct values here
        printf("%s\n", strings[i]);
    }

    for(int i = 0; i < SIZE; i++){
        //prints wrong values here
        printf("%s\n", strings[i]);
    }

    free(strings);
    return 0;
}

Upvotes: 0

Views: 79

Answers (3)

Lundin
Lundin

Reputation: 213563

There's actually no need to do this in run-time. You can do this at compile time if you can suffer some "X macro" tricks.

#define INIT_LIST \
   X(1) \
   X(2) \
   X(3) \
   X(4) \

#define STR(n) #n

#include <stdio.h>

int main (void)
{
  #define X(n) n,
    int nums[] = {INIT_LIST};
  #undef X

  #define X(n) STR(n),
    const char* str[] = {INIT_LIST};
  #undef X

  for(size_t i=0; i<sizeof(nums)/sizeof(*nums); i++)
  {
    printf("%d %s\n", nums[i], str[i]);
  }
}

Where INIT_LIST has to contain all the initializers as integers. After pre-processing, the above expands to this:

#include <stdio.h>

int main (void)
{
  int nums[] = {1,2,3,4};
  const char* str[] = {"1", "2", "3", "4"};

  for(size_t i=0; i<sizeof(nums)/sizeof(*nums); i++)
  {
    printf("%d %s\n", nums[i], str[i]);
  }
}

Performance-wise, this is naturally going to be astronomically faster than any solution using heap allocation. The down side is that X macros can be a bit hard to read.

Upvotes: 0

ggorlen
ggorlen

Reputation: 56875

The issue is strings[i] = temp;. This assigns temp to strings[i], but then temp is a local variable scoped to the loop block, and is not valid after the block terminates.

You'll need to malloc memory for each string to hold the copied value (and free it when done). tmp is also unnecessary since we can sprintf directly into the pointer.

SIZE = 5 but your array only has 4 members, so we have an out of bounds access. I'd prefer to scope this to the data it represents rather than make it a global constant. I'm also assuming this array will handle arbitrary data, because as-is, there's no advantage to it over using i + 1 inside your loop.

malloc(12) is sufficient space to hold a 32-bit int string (sizeof char is always 1 and we need space for '-' and '\0' characters). As pointed out in this comment, you can use sizeof(int) * CHAR_BIT / 3 + 2 to compute the correct size for the buffer where CHAR_BIT is defined in the limits.h header.

As an aside, there's no need to cast malloc and it's good practice to use sizeof(*strings) in case the pointer type changes during a refactor.

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

int main() {
    int nums[] = {1, 2, 3, 4};
    int nums_size = 4;
    char **strings = malloc(nums_size * sizeof(*strings));

    for (int i = 0; i < nums_size; i++) {
        strings[i] = malloc(12);
        sprintf(strings[i], "%d", nums[i]);
    }

    for (int i = 0; i < nums_size; i++) { 
        printf("%s\n", strings[i]);
        free(strings[i]);
    }

    free(strings);
    return 0;
}

Upvotes: 3

Steve Friedl
Steve Friedl

Reputation: 4247

Always let the compiler do counting for you.

First, I define below a macro COUNTOF that produces the number of items (not the number of bytes) in the array object; that should be used everywhere.

Second, the strdup() library function makes a copy of a string buffer - it counts the length, allocates the right number of bytes, then copies the string into it. Much easier than rolling your own.

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

#define COUNTOF(x)   ( sizeof(x) / sizeof((x)[0]) )

int main(){
    int nums[] = {1, 2, 3, 4};
    char **strings = malloc(COUNTOF(nums) * sizeof(*strings));

    for(int i = 0; i < COUNTOF(nums); i++){
        char temp[40];  // bigger than any plausible int
        sprintf(temp, "%d", nums[i]);
        strings[i] = strdup(temp);   // HERE
        printf("%s\n", strings[i]);
    }

    for(int i = 0; i < COUNTOF(nums); i++){
        //now prints correct value here :-)
        printf("%s\n", strings[i]);
    }

    return 0;
}

Upvotes: 2

Related Questions