Copying struct to array in C

Question

I am having a problem with creating a function that will add (copy) structure from parameter to resized array.

Here is my simplified code:

typedef struct User
{
    char FirstName[32];
    char LastName[32];
}
User;



User * Users
unsigned short UsersNum = 0;

void AddUser(User * NewUser)
{
    Users = (User *) realloc(Users, (UsersNum + 1) * sizeof(User));
    memcpy(Users[UsersNum], NewUser, sizeof(User));
    UsersNum++;
}

With this I'm getting:

error: incompatible type for argument 1 of `memcpy'

I've also tried

memcpy(Users + UsersNum, NewUser, sizeof(User));

But got SEGFAULT.

How can i copy NewUser to the end of Users?

Not sure you need the cast... generally, in C, you needn't cast a pointer returned by `malloc` & co — Elias Van Ootegem, Nov 29 '13 at 11:54

score 3 · Accepted Answer · edited May 23 '17 at 12:24

3

First, please don't cast the return value of malloc() in C.

Second, why do you feel you must use memcpy() here? The memcpy() function is great, but it's a rather blunt tool meant to "blindly" copy large areas of memory that have no further structure that the program is aware of. In your case, you know that the bytes you're copying make up an instance of the User type, i.e. it's simply a value of type User.

Values are typically moved around by assignment, so use that instead:

void AddUser(const User *NewUser)
{
    User *UsersGrown = realloc(Users, (UsersNum + 1) * sizeof *Users);
    if(UsersGrown != NULL)
    {
        Users = UsersGrown;
        Users[UsersNum] = *NewUser;
        ++UsersNum;
    }
}

You can see that I also made the argument const, and added some basic error-checking since realloc() can fail.

It would be a good idea to separate the allocated size of the Users array from the length of the array, so that you can grow them at different speeds and cut down on the number of calls to realloc(). In other words, allocate room for e.g. 8 elements at first, and when that is hit, double the size and realloc(), and so on. This gives much better performance if you expect many insertions.

edited May 23 '17 at 12:24

Community

1
1

answered Nov 29 '13 at 12:12

unwind

391,730
64
469
606

Great, but without the cast I'm getting: 9: warning: assignment makes pointer from integer without a cast – peku33 Nov 29 '13 at 12:53
I'm getting SEGFAULT from: Users[UsersNum] = *NewUser; – peku33 Nov 29 '13 at 12:58
@peku33 That warning implies you're missing `#include ` in your program, so there's no prototype for `realloc()`. See how much better everything becomes when you don't cast? – unwind Nov 29 '13 at 14:23
@peku33 No idea why you're getting a SEGFAULT, but I don't think it has anything to do with assignment over `memcpy()`. Run the code in a debugger. – unwind Nov 29 '13 at 14:24
Finally I found out that my method was stupid. I changed Users to array of pointers like in the anwser above. Now I don't have to copy anything, but I have to remember to clear memory. I suggest this method. It's much faster and easier. – peku33 Nov 30 '13 at 14:50

score 1 · Answer 2 · answered Nov 29 '13 at 11:55

1

You should ask yourself what type is User[UsersNum] ... that is User, and memcpy operates on pointers. To get address of it there is & operand. So it should be

memcpy(&Users[UsersNum], NewUser, sizeof(User));

Also please note the you are using very very inefficient way to implement the functionality. Copying/moving stuff around so much should be avoided.

answered Nov 29 '13 at 11:55

luk32

15,812
38
62

So what is the best way to store those records? – peku33 Nov 29 '13 at 12:05
There's nothing particularly inefficient here, or at least there is no way to tell, without knowing the bigger picture. – Lundin Nov 29 '13 at 12:05
Records are ok, but you do seem to reallocate whole vector just to add one element. Reallocation takes `O(n)` time, so the more record you will have the longer adding of a new one will take. The way they do it in `std::vector` I believe is to reserve some space ahead. So you have some empty slots ahead after relocation. How to choose how much space to reserve is not trivial. @Lundin I will be bold and say you are wrong. Moving whole vector to add one element is inefficient. Adding new element to a structure in `O(n)` time is considered slow and inefficient. – luk32 Nov 29 '13 at 12:16
That is equivalent to `memcpy(Users + UsersNum, NewUser, sizeof(User));` which the OP already tried. – cmaster - reinstate monica Nov 29 '13 at 12:26
Vectors are reserved to C++ and I have to use C. Thats the task. – peku33 Nov 29 '13 at 12:54
@peku33 I don't understand. I did not tell you to use it, but you are implementing essentially the same thing. I suggested to borrow idea from pros when you are doing similar things. – luk32 Nov 29 '13 at 13:12
@luk32 Ah you were referring to the realloc, not making the hard copy of the data. You said "copying/moving stuff around so much should be avoided" so I misunderstood and simply thought you meant copying from one object to another. The `realloc` part may indeed be quite slow. Some ADT based on linked lists would probably have been better in this case. – Lundin Nov 29 '13 at 16:01

Copying struct to array in C

2 Answers2