1

I am trying to dynamically initializing a queue. Here is my function.

typedef struct{
    int size;
    int max_size;
    short * eles;
} queue;

void dump_queue(queue *q)
{
    //print a bunch of information
}

void init_queue(queue *q, int max_size)
{
    q = (queue)malloc(sizeof(queue));
    q->size = 0;
    q->max_size = max_size;
    q->eles = (short *)malloc(max_size * sizeof(short));
    int i;
    for(i = 0; i < max_size; i++)
        q->eles[i] = -1;
    dump_queue(q);
}

task_queue is a global variable. The structure of the routine is like:(not exact code)

//globally defined here but not initialized 
queue * task_queue;
void init_scheduler()
{
    init_queue(task_queue, 32);

    dump_queue(task_queue);
    //other staff
}

note that there are two dump_queue, one is init_queue(), the other is after init_queue. Since task_queue is malloced, I expect that two calls of dump_queue should give a same result. But the second one actually reported a SIG_FAULT.

After I checked, in the second call of dump_queue, task_queue is actually NULL. Why is that?

Jongware
  • 22,200
  • 8
  • 54
  • 100
alvinzoo
  • 493
  • 2
  • 8
  • 17
  • You malloc some memory and then throw away the pointer to it. It's like putting money in your wallet, throwing *that* away, and then asking where your money went. ("It is still in your wallet" is the answer.) – Jongware Dec 03 '14 at 10:18
  • You cannot change a value at a different scope by assignment without indirection. `q = ...` will never work, you need `*q = ...`. – perreal Dec 03 '14 at 10:19
  • That won't work either @perreal. He needs to return the pointer or pass a `queue **` argument so he can assign the actual pointer wirh an address. The cleaner approach would be to locally declare `q` and return it, instead of passing it via argument using a pointer-to-pointer argument. – jweyrich Dec 03 '14 at 10:28
  • 2
    Please post the solution as an answer to your own question, rather than to edit the question to contain the answer as well. – Lundin Dec 03 '14 at 10:36
  • I rolled that last edit back; it invalidates the purpose of Stack Overflow as "a question and answer site", as stated in the [Introductory Tour](http://stackoverflow.com/tour). – Jongware Dec 03 '14 at 10:57

4 Answers4

5

Why not? you're allocating memory in the function local scope of init_queue().

The scope of the returned pointer is valid, but the assignment is not valid.

q = malloc(sizeof(queue));

this q won't hold it's value outside init_queue() function.

if queue * task_queue; is global, is it really required to pass it as function parameter?

Also, as a note, please do not cast the return value of malloc().

EDIT:

No, there is no auto-free() concept in c. It will result in a memory leak, if not free()-d by the application explicitly.

Community
  • 1
  • 1
Natasha Dutta
  • 3,242
  • 21
  • 23
  • Can you explain more specifically? Will the memory be auto freed after the function returned? – alvinzoo Dec 03 '14 at 10:18
  • especially, don't use an illegal cast on the return value of malloc! – M.M Dec 03 '14 at 10:19
  • Not the DV, but the memory allocated by `malloc` is not local to the function. You meant to say that the pointer to that memory is only stored in the local scope; so the calling code doesn't see it – M.M Dec 03 '14 at 10:21
  • thanks. That makes sense. I don't know where the down vote comes from. I will upvote you! – alvinzoo Dec 03 '14 at 10:22
  • @MattMcNabb yes, that's what I was trying to mean. the _assignment_ is local, not the returned pointer. – Natasha Dutta Dec 03 '14 at 10:32
2

You never assign anything to task_queue. Notice that you passed task_queue to init_queue by value instead of by reference. You should either modify init_queue to return a queue *, or modify its first argument to take a queue ** and pass in &task_queue from init_scheduler.

Or perhaps the easiest fix, since it is a global, just do task_queue = q; at the end of init_queue.

JS1
  • 4,745
  • 1
  • 14
  • 19
1

it should either be init_queue(&task_queue); or task_queue=init_queue();

Peter Miehle
  • 5,984
  • 2
  • 38
  • 55
0

if you use this function only to initialize task_queue you should do that

void init_global_queue(int max_size)
{
    queue *task_queue = malloc(sizeof(queue));
    task_queue->size = 0;
    task_queue->max_size = max_size;
    task_queue->eles = malloc(max_size * sizeof(short));
    int i;
    for(i = 0; i < max_size; i++)
        task_queue->eles[i] = -1;
}

But unfortunately, this is not a good pattern because you can't use this function more than once

I think the best way of doing it is like this

queue *init_queue(int max_size)
{
    queue *q = malloc(sizeof(queue));
    q->size = 0;
    q->max_size = max_size;
    q->eles = malloc(max_size * sizeof(short));
    int i;
    for(i = 0; i < max_size; i++)
        q->eles[i] = -1;
    return q;
}

and when you want to initialize any queue you'll just have to call this function

queue * task_queue;
void init_scheduler()
{
    task_queue = init_queue(32);

    dump_queue(task_queue);
}

And 2 things :

-Casting the return value of a malloc call is not really bad, in fact it's casted anyway, explicitly or implicitly

-The best way of doing a malloc and (for this example) returning its value is like this :

type *my_pointer = NULL;
my_pointer = malloc(sizeof (type))
if (my_pointer == NULL)
    return NULL;
my_pointer->foo = 0;
my_pointer->bar = NULL;
...
return my_pointer

So that if malloc doesn't work properly(i.e. for example : if your RAM is full, malloc will return NULL (and set errno to ENOMEM)), you can exit your program properly

and if you like minimalist programming, you can do it like this to ;)

type *my_pointer = NULL;
if (!(my_pointer = malloc(sizeof (type))))
    return NULL;

more ? man malloc, man errno

Charlon
  • 363
  • 2
  • 16