Why is my code not setting the variable?

Question

The purpose of the queue is to store pointers to objects of type T. The queue is used to push and pop objects on and off the queue as I traverse various data structures.

The queue works fine except the line :

out = t->payload;

(four lines from the bottom)

out never changes value to what is in t->payload. Looking at it in the debugger, I can see that t->payload is set correctly, but out is not assigned the value that is in t->payload. Can someone tell me what is going on?

template<typename T>
class Queue
{
private:
    struct node
    {
        T           *payload;
        struct node *next;
    };
    node *head;
    node *tail;
public:
    Queue()
    {
            head = new node;
        tail = new node;
        head->next = tail;
        tail->next = tail;
    }
        ...
    void push(T *payload)
    {
        node *newNode;
                node *temp;
        newNode = new node;
        temp = head->next;
        head->next = newNode;
        newNode->next = temp;
        newNode->payload = payload;
    }
    bool pop(T *out)
    {
        node *t;
        node *x;
        t = head;
        x = head;
        if(t == tail)
        {
            return false;  //already empty
        }
        while(t->next != tail)
        {
            x = t;
            t = t->next;
        }
        x->next = tail;
        out = t->payload;
        delete t;
        return true;
    }
}

Answer 1

bool pop(T *out)
T *payload;
out = t->payload;  

You are copying a T* to a T*, but the destination is a function parameter, which is invariably "in". Use a function result, or a T**out or a T * & out.

Answer 2

Strictly speaking, I cannot reproduce your error: out is being set to the value of t->payload... within the function.

bool pop(T *out)
{
  ...
  out = t->payload;
  ...
}

You pass in the pointer out by value. The function modifies the pointer out which is a local variable, not the thing it pointed to when you called the function, and not the pointer that may exist as a variable in the calling code.

Answer 3

out = t->payload;

This is the source of your problem. You are severing the link between the parameter passed to the function. Your parameter out is pointer. If you want to use this paradigm (note: not the best idea), you should never change out. You should instead change the contents of out via

*out = *(t->payload);

There are a two key issues with the above. This will croak if either out or t->payload is a null pointer. You can fix the former problem by passing a reference rather than a pointer. If you do that then you should use out = *(t->payload). This doesn't solve the problem of a null payload. You can fix that by ensuring that the payload is always non-null on adding a node.

A problem still remains, however. For example, what if that payload pointer is deleted, making dereferencing invalid? Using pointers is always a bit problematic.

Answer 4

Note that a container is better off if it does not force pointer types on the client. The very simple example given below avoids the explicit asterisk (*) in the signatures. Thus, it is open to the user whether she'll trust a Keeper with a plain type or a pointer type. Either way data is simply copied to and from the "keep" instance variable. If she decides to use pointers, fine, but the responsibilty for allocating and freeing dynamic memory rests with her.

template<typename T>
class Keeper {
public:
  Keeper() : keeping(false){}
  virtual ~Keeper(){}
  void push( T & payload ){
    if( keeping ) throw "I'm busy";
    keeping = true;
    keep = payload;
  }
  bool pop( T & payload ){
    if( ! keeping ) return false;
    keeping = false;
    payload = keep;
    return true;
  }
private:
  bool keeping;
  T keep;
};

Answer 5

Try adding some brackets when you create a new object.

Like this:

head = new node();
tail = new node();
...

For further explanation as to why this is important, see these links:

Do the parentheses after the type name make a difference with new?

Not using parentheses in constructor call with new (c++)

EDIT: Added a few print statements to your code. The problem seems to be twofold.

1. in push, instead of setting newNode->payload to payload, try setting it to new int(*payload). That way the value will be copied.
2. At the end of pop, you set the address of out to t->payload, but t is deleted right after, so out is pointing to nothing. Instead, copy the value as described above.

If you correct these two things, it seems to work!

Edit 2: Implementing it as a reference

bool pop(T &out)
{
    node *t;
    node *x;
    t = head;
    x = head;
    if(t == tail)
    {
        return false;  //already empty
    }
    while(t->next != tail)
    {   
        x = t;
        t = t->next;
    }
    x->next = tail;
    out = *(t->payload);
    delete t;
    return true;
}