tags:

views:

86

answers:

3

Hello,

gcc 4.4.4 c89

I have always thought of using malloc for the life of the project for being the scope.

But I am just wondering if my idea is the best practice. For example, I initalize an instance of the struct in main. And create 2 functions for creating and destroying. I am just wondering if this is the right thing to do.

I have some skeleton code below.

Many thanks for any advice,

typedef struct Network_dev_t {
    size_t id;
    char *name;
} Network_dev;

Network_dev* create_network_device(Network_dev *network)
{
    network = malloc(sizeof *network);
    if(network == NULL) {
        return NULL;
    }    
    return network;
}

void do_something(Network_dev *network)
{
    /* Do something with the network device */
}

void destroy_network_device(Network_dev *network)
{
    free(network);
}

int main(void)
{
    Network_dev *network = NULL;

    network = create_network_device(network);

    /* Do something with the network device */
    do_something(network);

    destroy_network_device(network);

    return 0;
}
+2  A: 
  • It is best to allocate memory and free memory in the same function. Just like you open and close files in the same function. You did this by creating and destroying a Network_dev in the main() function, which is good. This makes it easy to confirm that all malloced locations are also freed.
  • It is best to malloc() something as late as possible and free() it as soon as possible. That is, hold the memory for as short as possible. If your program's job is to do something with Network_dev, you did all right. If your program does a lot of other things, you should do them before malloc() or after free().
Sjoerd
I know this will be a *duh* for most folks, but about your second bullet, this doesn't mean you should move a malloc()/free() pair inside a tight loop just because you can.
T.E.D.
+4  A: 

Looks good.

I have a point or 2 about create_network_device

Network_dev* create_network_device(Network_dev *network)

no need to pass in a pointer; I'd rather have Network_dev* create_network_device(void)

{
    network = malloc(sizeof *network);

the if is not really necessary; if malloc failed the return network at the end of the function is the same as return NULL.

    if(network == NULL) {
        return NULL;
    }

If the allocation succeeded you might want to insure the struct members are in a know state here

    /* if (network) {       */
    /*     id = 0;          */
    /*     name = NULL;     */
    /* }                    */

    return network;
}
pmg
Yes, you are correct. Actually in my code I was initalizing the members. However, just to make it simple, I left that bit out. thanks.
robUK
+3  A: 

This code looks fine to me. I agree with pmg that your create_network_device could use a little work. Just to pull together what he said and make things clearer, here is exactly how I would write the function:

Network_dev *create_network_device()
{
    Network_dev *network = malloc(sizeof(*network));
    if (network) {
        network->id = 0;
        network->name = NULL;
    }
    return network;
}
Karmastan
+1 for cleaning up my sloppiness, thanks :)
pmg