C method for moving structs between dynamic arrays

Question

i have a card game in C (i have to make it in C, class project). And i created a method for moving Structs from one dynamic array to another. Unfortunately the method works at first, but later when i call it in the program it starts throwing malloc() errors. Deck is a struct that contains a int size and a dynamically allocated Card *deck, and Card is a struct for holding a single card. Is there something i am missing, or that has to be defined in another way ?

void moveCard(Deck *src, Deck *dest, int n){
    Card *tmpCards = NULL;
    Card tmp = (*src).deck[n];                          // get card to be copied
    int newSize = dest->size + 1;                       //increase dest size by 1
    tmpCards = (Card *) malloc(sizeof(Card) * newSize); // allocate new memory area
    memmove(&(tmpCards[0]), &(dest->deck[0]), sizeof(Card) * (newSize - 1)); 
        // copy all the cards from old memory area to new area
    dest->deck = tmpCards;  // reassign old pointer to new memory area
    dest->size = newSize;   // reevaluate size
    dest->deck[newSize - 1] = tmp; // copy card to end of new area


    tmpCards = NULL; // empty out pointer (no functionality, just for easier reading)
    newSize = src->size; // reevaluate newSize 
    tmpCards = (Card *) malloc(sizeof(Card) * (newSize - 1));
                     // allocate smaller mem area by 1
    memmove(&(tmpCards[0]),&(src->deck[0]), sizeof(Card) * (n));
                     // copy all cards up to the n-th point 
    memmove(&(tmpCards[n]),&(src->deck[n+1]), sizeof(Card) * (newSize - n));
                     // copy all cards from the n+1 -th point

    src->size = newSize -1; // reevaluate size
    src->deck = tmpCards;     // reassign pointer

    // since all input parameters were pointers, no return is necessary
}

Edit: to the people concerned by the fact that I don't release memory. The original code has 2 free commands in them (free(dest->deck) and free(src->deck)). but only one of them worked. The other created again memory corruption for some odd reason, so I found it better, not to put them into this example.

Point 1. Please [do not cast](http://stackoverflow.com/q/605845/2173917) the return value of `malloc()`. Point 2. Can't you use `realloc()`? — Sourav Ghosh, Dec 29 '14 at 06:07
Shouldn't you free the old `dest->deck` before assigning `tmpCards` to it? Or does it not yet point to anything? — Dmitri, Dec 29 '14 at 06:12
If you know the maximum number of cards any deck may contain, and it's not a ridiculously large number, you might consider a fixed size `deck` array where `size` indicates how much of it is used, rather than constantly reallocating memory every time a card is moved... it'd likely improve performance quite a bit. — Dmitri, Dec 29 '14 at 07:01
"The other created again memory corruption for some odd reason" - no, *you* created memory corruption, and `free()` merely alerted you to that fact. You need to fix that. — Crowman, Dec 29 '14 at 07:19
Errors in malloc can be caused by memory corruption in a completely different part of the program that was executed earlier. If you have a buffer overflow and write outside of your allocated memory, you can corrupt the malloc system's internal data structures, and get a crash much later. Therefore, a program fragment like this is sometimes not of much use when debugging. Use a debugger or something like valgrind to find your actual problem. — Thomas Padron-McCarthy, Dec 29 '14 at 07:22

R Sahu · Accepted Answer · 2014-12-29T17:15:09.590

I think you are running out of memory because you are not releasing any memory. You are only allocating new memory.

void moveCard(Deck *src, Deck *dest, int n){
    Card *tmpCards = NULL;
    Card tmp = (*src).deck[n];                          // get card to be copied
    int newSize = dest->size + 1;                       //increase dest size by 1
    tmpCards = (Card *) malloc(sizeof(Card) * newSize); // allocate new memory area
    memmove(&(tmpCards[0]), &(dest->deck[0]), sizeof(Card) * (newSize - 1)); 
        // copy all the cards from old memory area to new area

    // **** CORRECTION ****
    // Free previously allocated memory for dest->deck
    free(dest->deck);

    dest->deck = tmpCards;  // reassign old pointer to new memory area
    dest->size = newSize;   // reevaluate size
    dest->deck[newSize - 1] = tmp; // copy card to end of new area


    tmpCards = NULL; // empty out pointer (no functionality, just for easier reading)
    newSize = src->size; // reevaluate newSize 
    tmpCards = (Card *) malloc(sizeof(Card) * (newSize - 1));
                     // allocate smaller mem area by 1
    memmove(&(tmpCards[0]),&(src->deck[0]), sizeof(Card) * (n));
                     // copy all cards up to the n-th point 
    memmove(&(tmpCards[n]),&(src->deck[n+1]), sizeof(Card) * (newSize - n));
                     // copy all cards from the n+1 -th point

    src->size = newSize -1; // reevaluate size

    // **** CORRECTION ****
    // Free previously allocated memory for src->deck
    free(src->deck);

    src->deck = tmpCards;     // reassign pointer

    // since all input parameters were pointers, no return is necessary
}

PS You should use memcpy instead of memmove. memmove is the right function to use if the source and the destination overlap. memcpy is the right function to use if the source and the destination do not overlap.

I will try `memcpy`. Thx for the advise. If it works, then I'll highlight your answer — user3796577, Dec 29 '14 at 07:01
There is nothing wrong with using memmove even if the areas overlap. It could even be recommended, for uniformity and code robustness. — Thomas Padron-McCarthy, Dec 29 '14 at 07:18
@ThomasPadron-McCarthy, `memmove` is going to be more expensive than `memcpy` since the former uses a temporary buffer to first copy the data from the source and then copy it to the destination. That may or may not be an issue for the problem at hand. — R Sahu, Dec 29 '14 at 07:25
@RSahu: No, memmove does not use a temporary buffer. All it has to do is to check for overlap, and if found, start copying from the top or the bottom of the area depending on the type of overlap. For an example, look here: http://www.opensource.apple.com/source/ntp/ntp-13/ntp/libntp/memmove.c It _could_ use a temporary buffer instead, but it is not needed, and it would be a very inefficient way of doing it. — Thomas Padron-McCarthy, Dec 29 '14 at 07:27
@ThomasPadron-McCarthy, good point. It's not *necessary* for `memmove` to use a temporary buffer. — R Sahu, Dec 29 '14 at 17:14

C method for moving structs between dynamic arrays

1 Answers1

Linked