1

I have a vector type defined as follows:

typedef struct vector { void **items; unsigned int capacity; unsigned int size; } vector_st; typedef vector_st *vector_t;

And I allocate and free it as follows:

vector_t vector_init(unsigned int capacity) { vector_t v = (vector_t)calloc(1, sizeof(vector_st)); v->capacity = capacity; v->size = 0; v->items = malloc(sizeof(void *) * v->capacity); return v; } void vector_free(vector_t v) { if (v) { free(v->items); v->capacity = 0; v->size = 0; free(v); v = NULL; } }

Now, the point is that I want to copy one vector to another one, meaning including all its content. So, I tried to define a function like this:

void vector_copy(vector_t to, vector_t from) { memcpy(to, from, sizeof(vector_st)); }

But, this does not seem to work quite right, as when I do something like this:

vector_t vec = vector_init(3); vector_add(vec, 1); vector_add(vec, 2); vector_add(vec, 3); unsigned int i; for (i = 0; i < vec->size; i++) { printf("%d\n", (int)vector_get(vec, i)); } vector_t copied = vector_init(3); vector_copy(copied, vec); vector_free(vec); for (i = 0; i < copied->size; i++) { printf("%d\n", (int)vector_get(copied, i)); }

For the first vector it correctly prints 1 2 3 but for the second one it prints 0 2 3. So, basically I believe it just copies maybe the memory addresses and not the actual content, as when I free the first vector the first element is set to 0. Any ideas how to copy this structure in my case?

EDIT:

void vector_resize(vector_t v, unsigned int capacity) { void **items = realloc(v->items, sizeof(void *) * capacity); if (items) { v->items = items; v->capacity = capacity; } } void vector_add(vector_t v, void *item) { if (v->capacity == v->size) { vector_resize(v, v->capacity * 2); } v->items[v->size++] = item; }
Improve this question
8
  • memcpy(to, from, sizeof(vector_st)); What's wrong with *to = *from;? Commented Sep 14, 2018 at 21:14
  • You have the same problem with the memcpy(). Commented Sep 14, 2018 at 21:17
  • @melpomene Because it will produce the same result I have as with memcpy(), and that is not the intended result. As Richard pointed out above. Commented Sep 14, 2018 at 21:19
  • Can you paste the code to vector_add? Also, what do you mean by "copies the memory addresses and not the actual content"? From the code you've shown us, the only content in the vector is a collection of memory addresses. Commented Sep 14, 2018 at 21:22
  • Because you are using pointers you have a shallow copy versus deep copy decision to make. When you do the copy do you just copy the pointers over or do you make a copy of the data pointed to? Currently you do a shallow copy which results in memory leak on the copy as well as when you free() the object being copied, you also free() the copied to pointers. Commented Sep 14, 2018 at 21:22

4 Answers 4

Reset to default
2

Now, the point is that I want to copy one vector to another one, meaning including all its content.

What you seem to want to perform is called a "deep copy". That means copying not just the data, but any additional pointed-to data, recursively. You have judged correctly that memcpy() of the structure itself does not do this; the vector elements are pointed to by pointers in the structure, but they themselves are elsewhere, and therefore are not copied.

What's worse, you have a fundamental problem here with copying the pointed-to data: your copy function doesn't know how big the pointed-to elements are. Without such knowledge, it is impossible to copy them. Furthermore, if the elements themselves contain pointers, then to perform a true deep copy, you need information about which members those are, and how large are the objects to which they point. Etc.

Basically, then, it is impossible write a generic deep copy (in any language). Performing a deep copy requires information at every level about what you are copying. To give you a bit of a flavor, however, you could copy one level deeper if you could rely on the vector elements being of a consistent size that is known at call time. That might look something like this:

void vector_copy(vector_t to, vector_t from, size_t element_size) { // NOTE: robust code would check for memory allocation failures. This code does not. void **temp_items; to->capacity = from->capacity; to->size = from->size; // evaluates to NULL if allocation fails: temp_items = realloc(to->items, from->capacity * sizeof(*to->items)); to->items = temp_items; for (int i = 0; i < from->size; i++) { to->items[i] = malloc(element_size); // evaluates to NULL if allocation fails memcpy(to->items[i], from->items[i], element_size); } }

You could avoid the need to pass the element size by instead making it a member of the vector structure.

Note that this assumes that vector to has been initialized, and that it is not necessary to free the pointers to the individual items, if any, that are currently in it (i.e. the vector does not own these, and is therefore not responsible for managing their memory).

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

3 Comments

The leaks the contents to->items from before the copy.
Yes, I assume the space is already allocated, so it is only intended to be a state change operation.
Ok, @DavidSchwartz, the primary memory leak is fixed, and I've added some commentary about managing the memory for the vector elements.
1 
void vector_copy(vector_t to, vector_t from) { memcpy(to, from, sizeof(vector_st)); }

This is wrong because the vector owns its items object. So you need to do four things in vector_copy:

1) Free the existing items objects in the destination vector so that it's not leaked.
2) Copy the capacity and size.
3) Allocate a brand new items for the destination vector to own.
4) Copy the source items into the newly-allocated destination items.

If you consider vector_copy to be an initialization function, skip step 1. But in that case, I'd strongly suggest changing the name to vector_init so that it's clear that it creates a new vector.

Improve this answer

2 Comments

No, it's not supposed to be an initialization function. Just state change.
@terett In that case, you can consolidate steps 1 and 3 into a resize operation.
1

You're simply trying to access free()d memory. First you're calling free(v->items) in vector_free(), and then you're trying to print its contents as if nothing had happened!

When you copy a vector to another (no matter how) the new vector will hold a reference to old_vector->items, so you cannot just free() it -- C is not a garbage collected language and malloc doesn't reference-count the blocks it's managing.

Improve this answer

Comments

1

So, basically I believe it just copies maybe the memory addresses and not the actual content

Yes, because at this line

memcpy(to, from, sizeof(vector_st));

you simply discards whatever memory was allocated for to, so memory held by copied becomes leaked; and after vector_free(vec) copied now holds a dangling reference to an already-freed memory.

To copy vector's contents into another, well, copy it:

memcpy(to->items, from->items, sizeof(void *) * from->size);

(Or less efficient

int i; for(i = 0; i < from->size; ++i) to->items[i] = from->items[i];

)

Or, better, define a copy-constructor:

vector_t vector_copy(vector_t src); // this functions allocates a fresh vector on heap; // set its size and capacity to those of src; // sets its items to a fresh array on heap of capacity elements; // copies src's items into that array; // and returns it
Improve this answer

3 Comments

The OP appears to have fooled you: vector_t is already a pointer type, and there does not appear to be any need for an additional level of indirection.
@JohnBollinger I had thought the same thing until I saw this comment. typedefs for pointers are really misleading when glancing at function signatures.
@JohnBollinger Thank you, edited. Long posts are hard to follow.