Even the Copy Assignment Operator can't assist

Question

Kindly help me figure out where the issue is. I have followed the rule of three as well and made several modifications to the code.

#include <iostream>

using namespace std;

class AStack {
    public:
        AStack();
        AStack(int);
        AStack(const AStack&);
        ~AStack();
        AStack& operator = (const AStack& s);
        void push(int);
        int pop();
        int top();
        bool isEmpty();
        void flush();

    private:
        int capacity ;
        int* a;
        int index = -1; // Index of the top most element
};

AStack::AStack() {
    a = new int[25];
    capacity = 25;
}

AStack::AStack(int size) {
    a = new int[size];
    capacity = size;
}

AStack::AStack(const AStack& s) {
    capacity = s.capacity;
    delete[] a; // To avoid memory leak
    a = new int[capacity];
    for (int i = 0; i < capacity; i++) { 
        a[i] = s.a[i];
    }
    index = s.index;
}

AStack::~AStack() {
    delete[] a;
}

AStack& AStack::operator = (const AStack& s) {
    capacity = s.capacity;
    delete[] a; // To avoid memory leak
    int* a = new int[capacity];
    for (int i = 0; i < capacity; i++) {
        a[i] = s.a[i];
    }
    index = s.index;
    return *this;
}

void AStack::push(int x) {
    if (index == capacity - 1) {
        cout << "\n\nThe stack is full. Couldn't insert " << x << "\n\n";
        return;
    }
    a[++index] = x;
}

int AStack::pop() {
    if (index == -1) {
        cout << "\n\nNo elements to pop\n\n";
        return -1;
    }
    return a[index--];
}

int AStack::top() {
    if (index == -1) {
        cout << "\n\nNo elements in the Stack\n\n";
        return -1;
    }
    return a[index];
}

bool AStack::isEmpty() {
    return (index == -1);
}

void AStack::flush() {
    if (index == -1) {
        cout << "\n\nNo elements in the Stack to flush\n\n";
        return;
    }
    cout << "\n\nFlushing the Stack:  ";
    while (index != -1) {
        cout << a[index--] << "  ";
    }
    cout << endl << endl;
}

AStack& reverseStack(AStack& s1) {
    AStack s2;
    while (!s1.isEmpty()) {
        s2.push(s1.pop());
    }
    s1 = s2;
    return s1;
}

int main() {

    AStack s1;
    s1.push(1);
    s1.push(2);
    s1.push(3);
    s1.push(4);
    s1.push(5);
    s1 = reverseStack(s1);
    cout << "\n\nFlushing s1:\n";
    s1.flush();
    system("pause");
    return 0;
}

I fail to understand how even after defining the appropriate copy assignment operator, the values in s1 after returning from the function are garbage values.

Answer 1

If your copy constructor is correct, and your destructor is correct, your assignment operator could be written in a much easier and safer fashion.

Currently, your assignment operator has two major flaws:

1. No check for self-assignment.
2. Changing the state of this before you know you can successfully allocate the memory (your code is not exception safe).

The reason for your error is that the call to reverseStack returns a reference to the current object. This invoked the assignment operator, thus your assignment operator was assigning the current object to the current object. Thus issue 1 above gets triggered.

You delete yourself, and then you reallocate yourself, but where did you get the values from in the loop to assign? They were deleted, thus they're garbage.

For item 2 above, these lines change this before you allocate memory:

capacity = s.capacity;
delete[] a; // To avoid memory leak

What happens if the call to new[] throws an exception? You've messed up your object by not only changing the capacity value, but you've also destroyed your data in the object by calling delete[] prematurely.

The other issue (which needs to be fixed to use the copy/swap idiom later in the answer), is that your copy constructor is deallocating memory it never allocated:

AStack::AStack(const AStack& s) {
    capacity = s.capacity;
    delete[] a;  // ??  What

Remove the line with the delete[] a, since you are more than likely calling delete[] on a pointer that's pointing to garbage.

Now, to rid you of these issues with the assignment operator, the copy/swap idiom should be used. This requires a working copy constructor and a working destructor before you can utilize this method. That's why we needed to fix your copy constructor first before proceeding.

#include <algorithm>
//...
AStack& AStack::operator = (AStack s) 
{
    std::swap(capacity, s.capacity);
    std::swap(a, s.a);
    std::swap(index, s.index);
    return *this;
}

Note that we do not need to check for self assignment, as the object that is passed by value is a brand new, temporary object that we are taking the values from, and never the current object's values (again, this is the reason for your original code to fail).

Also, if new[] threw an exception, it would have been thrown on the call to the assignment operator when creating the temporary object that is passed by value. Thus we never get the chance to inadvertently mess up our object because of new[] throwing an exception.

Please read up on what the copy/swap idiom is, and why this is the easiest, safest, and robust way to write an assignment operator. This answer explains in detail what you need to know:

What is the copy-and-swap idiom?

Here is a live example of the fixed code. Note that there are other changes, such as removing the default constructor and making the Attack(int) destructor take a default parameter of 25.

Live Example: http://ideone.com/KbA20D