Assigning a variable to dynamically allocated return value?

Question

I am trying to assign a variable to a return value but the deconstructor is called before the values are assigned to pq. I also get the following error which I'm assuming is an attempt to double delete a variable:

_BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

    PriorityQueue pq;
    pq = FindThroughPaths(a, b);

FindThroughPaths(a, b) returns a PriorityQueue.

I may be getting this error because the return value has dynamically allocated member variables.

I am simply trying to find a way to keep the return value of FindThroughPaths(a, b) within the scope and I do not want to create multiple instances just to access it's members. This is an example of what I would like:

    PriorityQueue pq;
    pq = FindThroughPaths(a, b);
    while (!pq.IsEmpty) {
         Path tmp = pq.Dequeue();
         std::cout << "\n" << tmp.name << " #" << tmp.number;
    }

The constructor, deconstructor, and assignment operator for PriorityQueue:

    PriorityQueue::PriorityQueue()
    {
            std::cout << "\nConstructor called." << endl;
            items.elements = new ItemType[maxItems];
            length = 0;
    }


    PriorityQueue::~PriorityQueue()
    {
           std::cout << "\nDeconstructor called." << endl;
           delete[] items.elements;
    }

    PriorityQueue PriorityQueue::operator=(const PriorityQueue &originalPQ)
    {
           //This function is started after the return value, the right operand, has been deconstructed.
           std::cout << "\nAssignment called." << endl;
           for (int i = 0; i < maxItems; i++)
                   items.elements[i] = originalPQ.items.elements[i];
           return *this;
     }

Using `new` and `delete` is a big red flag. To use them properly you need to know about the [Rule of Five](http://en.cppreference.com/w/cpp/language/rule_of_three). But the better approach is to use the Rule of Zero, and store your data in a container that manages its own memory. For example, `items.elements` could be a `vector`. — M.M, Sep 26 '15 at 04:20
The assignment operator should return by reference. (But if you follow the Rule of Zero then you do not need assignment operator,destructor, or copy constructor). — M.M, Sep 26 '15 at 04:22

score 0 · Answer 1 · edited May 23 '17 at 11:44

It is a little hard to tell exactly what the problem is without looking at how you are returning the PriorityQueue from your FindThroughPaths method, but it is likely that the problem is that you are missing a copy constructor in the PriorityQueue class.

PriorityQueue::PriorityQueue(const PriorityQueue &other)
{
    std::cout << "\nCopy constructor was called" << endl;
    // there are better ways to implement this, but I think this will correctly
    // defer to the assignment operator you wrote
    *this = other;
}

When you return an object from a method, the copy constructor is invoked to build the object that will be returned. If you have not implemented a copy constructor, then a default implementation is used, and that default will not be sufficient in your case.

The destructor will still be called to destroy the PriorityQueue object that is local to the FindThroughPaths method.

You might find this other question about returning an object from a method to be helpful.

Yes I did forget a copy constructor but for some reason that code did not work. It caused an infinite loop of the assignment operator and copy constructor calling each other. I got it to work by taking out the assignment operator and just putting it's contents into the copy constructor, with an additional memory allocation at the start. — michael874, Sep 26 '15 at 03:48
I didn't notice earlier, but the operator= is usually defined to return by reference as in `PriorityQueue& PriorityQueue::operator=`. If you did that, it might avoid the infinite loop. And, yes, I forgot to allocate the memory at the top of the copy constructor. — Evan VanderZee, Sep 26 '15 at 04:03

Assigning a variable to dynamically allocated return value?

1 Answers1