0

I was doing some sort of infix-to-postfix conversion.

My program works fine but at the end it gives some sort of error!

Error Info

I guess the error is on the void printQueueContents(Queue typeInfo) method of following class

template <class T>
class Queue{
private:
    int front, rear, max;
    T *q;
public:
    Queue(int size=0){
        front = rear = 0;
        max = size;
        q = new T[size];
    }
    ~Queue(){
        delete []q;
    }
    int enqueue(T);
    T dequeue();

    T getValueFromIndex(int index){
        return q[index];
    }

    void printQueueContents(Queue typeInfo){
        for(int i=rear; i<front;i++){
            if(typeInfo.getValueFromIndex(i)) {
            cout<<q[i]<<" ";
            } else {
                cout<<static_cast<char>(q[i])<<" ";
            }
        }
        cout<<"\n";
    }
};

which is called as: 

q1.printQueueContents(q2);

And declared as:

Queue<int> q1(200);
Queue<int> q2(200); // Holds data type information for q1

I cannot figure out why is this happening? Any suggestions?

EDIT: Thanks to peoples with answers. I finally managed to solve it by changing the method to: 

void printQueueContents(Queue& typeInfo)

so that while calling this function the default copy constructor does not make a copy and destructor does not accidently deletes the same memory location twice. Here's a link that has a intuitive explaination. http://www.drdobbs.com/c-made-easier-the-rule-of-three/184401400

cipher
  • 2,414
  • 4
  • 30
  • 54
  • 4
    possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – juanchopanza Jan 14 '14 at 17:36
  • 4
    I don't think this counts as a duplicate of that question, as the OP is getting a specific (searchable) error not mentioned in that post. That the solution to this problem is to follow that rule deserves a mention/link (as in the answer below) but doesn't warrant closing as duplicate. – Reinstate Monica -- notmaynard Jan 14 '14 at 17:53

2 Answers2

2

While it is true that since you declared a destructor you should declare copy assignment operator and copy constructor(rule of three). You might not want to have them public though.

Calling a function that takes your class by value will invoke the copy constructor. You rely on the compiler generated copy constructor if you don't declare any and it does member to member assignment. In your case, the q* gets copied as is to your second object. When returning from the function, the parameter passed by value goes out of scope and get destructed which will delete q for the first time. When your program exits, the destructor of the original object gets called as well so q is deleted a second time.

In your case, I think the printQueueContents method should take a const &(or ref) to your Queue instead to avoid (possibly) expensive copy of the Queue object. Same could be said for getValueFromIndex that should return a const & or ref to avoid copy. For int this is not a problem but for classes, it could get expensive quickly.

With that being said, you still have to either implemented copy assignment operator and copy constructor or declared them as private.

Eric Fortin
  • 7,533
  • 2
  • 25
  • 33
  • I'm actually new to C++ and do not "exactly" know about copy assignment operator and copy constructor. I'm getting confused how to implement those. Any links would be great! – cipher Jan 14 '14 at 17:56
  • Thanks. I just used the object in the argument by ref and it solved. And thank you for the explaination! – cipher Jan 15 '14 at 18:34
1

You must implement copy constructor and copy assignment operator

Queue(const Queue& rop){
 // copy rop to this, means allocate local q buffer, and copy contents of rop.q to it
}

Queue& operator=(const Queue& rop){
 // update this with rop, means the same as above
 return *this;
}

your class must obey rule of three, mostly because your class is using bare pointer to allocated buffer. You could also use shared_ptr for your buffer, it would manage life time of it.

marcinj
  • 48,511
  • 9
  • 79
  • 100
  • `//copy rop to this` does this mean `this = rop;`. And what do `//update this with rop` means? – cipher Jan 14 '14 at 17:44
  • added some more info, also here is a good explanation on how to implement those two methods: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – marcinj Jan 14 '14 at 17:47
  • honestly, I dont remember when I last time allocated using `new[]`, and I do c++ for living. Unleas you are learning new things, You should really switch to stl::vector for array allocations. This will save you from worrying about memory leaks and segfaults. I have not seen those in my code for a very long time. – marcinj Jan 14 '14 at 17:49
  • I am actually learning Cpp (had some C backgrounds), and have not been upto STL yet. Thanks for the answer though! :) – cipher Jan 14 '14 at 17:52