2

I'm working in C

I have a struct called Entity and I create a dynamic array of that struct. Then I try to remove one element from the array but I don't get the behaviour I want.

Here is the code I'm using:

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

typedef struct Entity
{
    int x, y;
    int velX, velY;
}Entity;

int remove_element(Entity** array, int sizeOfArray, int indexToRemove)
{
    int i;

    printf("Beginning processing. Array is currently: ");
    for (i = 0; i < sizeOfArray; ++i)
        printf("%d ", (*array)[i].x);
    printf("\n");

    Entity* temp = malloc((sizeOfArray - 1) * sizeof(Entity)); // allocate an array with a size 1 less than the current one

    memmove(
            temp,
            *array,
            (indexToRemove+1)*sizeof(Entity)); // copy everything BEFORE the index

    memmove(
            temp+indexToRemove,
            (*array)+(indexToRemove+1),
            (sizeOfArray - indexToRemove)*sizeof(Entity)); // copy everything AFTER the index


    printf("Processing done. Array is currently: ");
    for (i = 0; i < sizeOfArray - 1; ++i)
        printf("%d ", (temp)[i].x);
    printf("\n");

    free (*array);
    *array = temp;
    return 0;

}

int main()
{
    int i;
    int howMany = 20;

    Entity* test = malloc(howMany * sizeof(Entity*));

    for (i = 0; i < howMany; ++i)
        (test[i].x) = i;

    remove_element(&test, howMany, 14);
    --howMany;

    return 0;
}

And the output I get :

Beginning processing. Array is currently: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
Processing done. Array is currently: 0 1 2 3 4 1866386284 6 7 8 9 10 11 12 13 15 16 17 18 19

Then the program crashes at the free (*array); line. I want my second line to be 0 1 2 3 4 5 6 7 8 9 10 11 12 13 15 16 17 18 19.

How could I solve my problem ?

Improve this question
12
  • Undefined behavior Commented Jan 20, 2018 at 13:51
  • At what point exactly is the content faulty? It's the line before that which causes the bug. BTW: The point of memmove() is that source and target can overlap, since you copy to new memory, you could use memcpy(). Commented Jan 20, 2018 at 13:56
  • Consider what happens if indexToRemove is zero. The first memmove() copies one data structure (when it should copy none) and the second copies sizeOfArray structures to a buffer/array that contains sizeOfArray-1 such structures. The behaviour is therefore undefined, even in that simple case. In short: you need to check your bounds better. Commented Jan 20, 2018 at 13:57
  • @Peter I simply added if(indexToRemove > 0) before the first memmove(), and if(indexToRemove < sizeOfArray - 1) before the second one, that should do it for the bounds ? Commented Jan 20, 2018 at 14:04
  • @Drakalex.: I have added an edit. If you were using 0 indexing should follow these Commented Jan 20, 2018 at 14:07

4 Answers 4

Reset to default
2

First thing you have allocated memory space for holding 20 Enity*. Then you have dereferenced it (and the value it contained is indeterminate). This is undefined behavior. And all story ends here.

But let's analyze what you mostly wanted.

Entity* test = malloc(howMany * sizeof(Entity));
                                       ^^^^^^^

is what you wanted. Because only if you do this you will get the member elements x and so on.

Also if you are considering 0 indexing then the memmove calls should be

memmove(temp, *array, (indexToRemove)*sizeof(Entity)); 
memmove(temp+indexToRemove, (*array)+(indexToRemove+1), 
            (sizeOfArray - indexToRemove - 1)*sizeof(Entity));

These two changes will be enough to solve the problems you are facing and realizing the correct behavior. (If this is all there is in your code).

Also as per standard the main() should be declared like this in case it doesn't take any parameter int main(void). Free the dynamically allocated memory when you are done working with it. Also you should check the return value of malloc - in case it fails it returns NULL and you should handle that case.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

This solved the first problem, my new array is what I wanted, but it still crashes at the free (*array); line
In your second memmove, shouldn't it be temp+indexToRemove*sizeof(Entity) (and similar with the second argument)?
@StephanLechner.: No it's not needed....pointer arithmetic is dictated by what it points to ...so it will move that much block itself.
@coderredoc Thanks you it finally work ! What did you change ?
@Drakalex.: Your earlier code had off by one error in the second memmove also. That's what I corrected. (sizeOfArray - indexToRemove ) --> (sizeOfArray - indexToRemove - 1)
|
0

Your offset calculations are off by one in both memmove instances. Use this instead:

// copy everything BEFORE the index
memmove(temp,
        *array,
        indexToRemove * sizeof(Entity));

// copy everything AFTER the index
memmove(temp + indexToRemove,
        *array + indexToRemove + 1,
        (sizeOfArray - indexToRemove - 1) * sizeof(Entity));
Improve this answer

Comments

0

In main itself your memeory allocation is not done properly.if you are using double pointer you should allocate memory first for double pointer and than single pointer in loop one by one.

Improve this answer

4 Comments

Not a double pointer (Entity* test = ...) then the address of test is passed to remove_element.
Entity* test = malloc(howMany * sizeof(Entity*)); for (i = 0; i < howMany; ++i) (test[i].x) = i;//acessing without allocating memory. run with valgrind invalid read/write will come
Exactly... How many '*' do you see between Entity and test?
You are allocating memory by multiplying size of *Entity and size of entity always be 4 byte since it is address .Not able to uderstand whether idea to allocate memory for structure element or structure pointer.Currently this code is invalid.
0

a little touch

remove element in any type of struct array

regards

int remove_element(void **input_ptr, int input_size, int index_remove, int struct_size)
{
    void *temp_ptr;

    temp_ptr = malloc((input_size - 1) * struct_size);
    if (temp_ptr == 0)
        return -1;

    memmove(temp_ptr, *input_ptr, index_remove * struct_size);

    memmove(temp_ptr + (index_remove * struct_size), (*input_ptr) + (index_remove + 1) * struct_size, (input_size - index_remove - 1) * struct_size);

    free(*input_ptr);

    *input_ptr = temp_ptr;

    return 1;
}

usage example for question struct

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

typedef struct Entity
{
    int x, y;
    int velX, velY;
}Entity;

int remove_element(void **input_ptr, int input_size, int index_remove, int struct_size)
{
    void *temp_ptr;

    temp_ptr = malloc((input_size - 1) * struct_size);
    if (temp_ptr == 0)
        return -1;

    memmove(temp_ptr, *input_ptr, index_remove * struct_size);

    memmove(temp_ptr + (index_remove * struct_size), (*input_ptr) + (index_remove + 1) * struct_size, (input_size - index_remove - 1) * struct_size);

    free(*input_ptr);

    *input_ptr = temp_ptr;

    return 1;
}

int main()
{
    int i;
    int howMany = 20;

    Entity* test = malloc(howMany * sizeof(Entity));

    for (i = 0; i < howMany; ++i)
    {
        (test[i].x) = i;
        printf("test[%d].x = '%d'\n", i, test[i].x);
    }

    remove_element((void**)&test, howMany, 14, sizeof(Entity));
    --howMany;

    printf("Deleted index --- new array\n");
    for (i = 0; i < howMany; ++i)
        printf("test[%d].x = '%d'\n", i, test[i].x);

    return 0;
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.