1

I have defined a stack class containing methods for pushing and popping values onto the stack.

In the tester file (shown below), after running it, an occur occurs & the program crashes. I know this is due to the function f, which creates an error as two pointers are pointing to the same location in memory. If i comment out the line f(s) when the function is called, the pop & push functions work correctly and the output is correct.

To fix this error, I have been asked to ; Create a copy constructor for this class to fix the above problem.

I'm not really familiar with this, so any help would be appreciated in how to do this. Thanks

Main Test file

#include "Stack.h"
#include <iostream>
#include <string>
using namespace std;

void f(Stack &a) {
    Stack b = a;
}


int main() {

    Stack s(2); //declare a stack object s which can store 2 ints
    s.push(4); //add int 4 into stack s

    //s = [4]
    s.push(13); //add int 13 into stack s
    //s = [4,13]

    f(s); //calls the function f which takes in parameter Stack a , and sets Stack b = to it.
    //error here - as 2 pointers point to the same location in memory !
    cout << s.pop() << endl; //print out top element(most recently pushed) element.
    //so should output 13
    return 0;
}

Header File Code

#ifndef STACK_H
#define STACK_H

class Stack {
public:
    //constructor
    Stack(int size);

    //destructor
    ~Stack();

    //public members (data & functions)
    void push(int i);
    int pop();

private:
    //private members (data & functions)
    int stck_size;
    int* stck;
    int top;
};

#endif

Stack.cpp Code

#include "Stack.h"
#include <iostream>
#include <string>
using namespace std;

Stack::Stack(int size){
    stck_size = size;
    stck = new int[stck_size];
    top = 0;
}
Stack::~Stack() {
    delete[] stck;
}
void Stack::push(int i) {
    if (top == stck_size) {
        cout << "Stack overflow." << endl;
        return;
    }
    stck[top++] = i;
}

int Stack::pop() {
    if (top == 0) {
        cout << "Stack underflow." << endl;
        return 0;
    }
    top--; //decrement top so it points to the last element istead of the empty space at the top.
    return stck[top];
}
Liam
  • 429
  • 1
  • 13
  • 33
  • The error is probably in your copy constructor (in Stack.h). – Franck Nov 13 '16 at 22:49
  • Included the stack.h code above. I don't actually have a copy constructor as of yet? this is what its asking me to create. – Liam Nov 13 '16 at 22:50
  • You need to provide us the content of Stack.h (at least) and Stack.cpp (preferably) in order to get any help. How we can help you implementing copy ctor without knowledge about Stack implementation? – woockashek Nov 13 '16 at 22:51
  • What you have run afoul of is the Rule of Three. [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) Click the link and find out. It will save you tonnes of debugging. – user4581301 Nov 13 '16 at 22:51
  • Not only do you need to implement a copy constructor, but also the assignment operator. – Sam Varshavchik Nov 13 '16 at 22:58

2 Answers2

1

Copy constructor here is pretty quick-and dirty:

Stack::Stack(const Stack & src): 
    stck_size(src.stack_size),
    stck(new int[stck_size]),
    top(src.top) //Member Initializer List
{
    // copy source's stack into this one. Could also use std::copy.
    // avoid stuff like memcpy. It works here, but not with anything more 
    // complicated. memcpy is a habit it's just best not to get into
    for (int index = 0; index < top; index++)
    {
        stck[index] = src.stck[index];
    }
}

Now that you have a copy constructor, you're still likely screwed because the Rule of Three has not been satisfied. You need operator=. And this is easy because the copy construct and the copy and swap idiom makes it easy.

Basic form:

TYPE& TYPE::operator=(TYPE rhs) //the object to be copied is passed by value
                                // the copy constructor makes the copy for us.
{
  swap(rhs); // need to implement a swap method. You probably need one 
             //for sorting anyway, so no loss.
  return *this; // return reference to new object
}
Community
  • 1
  • 1
user4581301
  • 33,082
  • 7
  • 33
  • 54
  • swap is not appropriate in an assignment operator. Nor should the right hand side be modifiable. – MarcD Nov 14 '16 at 03:28
  • @MarcD I need to see a LOT of backing for both of those claims. I'll start defending my side of the argument by hiding behind [Herb Sutter](http://www.gotw.ca/gotw/059.htm) and [Scott Meyers](http://scottmeyers.blogspot.ca/2014/06/the-drawbacks-of-implementing-move.html) – user4581301 Nov 14 '16 at 04:40
  • Your not implementing the idiom as herb sutter describes it. If you were implementing a move fine but you're not. Second, since when does an assignment modify the right hand side??? That was is not expected nor is it desired behavior. If you'll pay attention to the example he gives he swaps with a temporary copy of the object, not the object itself. – MarcD Nov 14 '16 at 05:23
  • @MarcD when you pass `rhs` into `operator=` by value it makes a temporary copy. The Sutter example is probably a bit more canonical by using the `const` reference, but at the end it does the exact same thing. It makes a copy, then swaps. As for the Meyers link I apologize. I linked the wrong page and have to withdraw that example. I don't have a good replacement at this time. As for returning the reference, you need it to get `A=B=C;` chaining. – user4581301 Nov 14 '16 at 05:34
  • Semantically you're incorrect because you're invalidating every single normal assignment the class makes by making it swap contents. – MarcD Nov 14 '16 at 05:36
  • But I still don't see the point of making a copy when you don't need one – MarcD Nov 14 '16 at 05:40
  • @MarcD The assignment makes A into a copy of B. The copy construction making a temporary copy of B does all of the copying for us, now all we need to do is exchange the guts of Copy of B with the guts of A. After the swap, A is now a duplicate of B and A's old guts are in copy of A where they will be destroyed correctly when Copy of A goes out of scope. [In-depth discussion of what's going on and why](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) – user4581301 Nov 14 '16 at 05:53
0

Your copy constructor should look like this:

Stack::Stack(const Stack &r) {
    stck_size = r.stck_size;
    stck = new int[stck_size];
    top = r.top;
    memcpy(stck, r.stck, top*sizeof (int));
}
woockashek
  • 1,588
  • 10
  • 25
  • What #include do I need in order for memcpy to work? giving me error saying identifier not found – Liam Nov 13 '16 at 23:08
  • `#include ` – woockashek Nov 13 '16 at 23:10
  • 1
    @LiamLaverty Don't use memcpy. It will just cause you this exact problem later. `memcpy` is awesomely stupid. It copies from A to B without any though to whether it should or not. This is exactly what copy constructors and assignment operators are for and `memcpy` doesn't know how to use them. [Use `std::copy`](http://en.cppreference.com/w/cpp/algorithm/copy) or a `for` loop. – user4581301 Nov 13 '16 at 23:16
  • As for this answer, code-only answers lead to [Cargo Cult programmers](https://en.wikipedia.org/wiki/Cargo_cult_programming). It can be salvages with some explanations of what you did and why. – user4581301 Nov 13 '16 at 23:17
  • @user4581301: Ok, I'll keep that in mind. I started posting here just few days ago so thanks for the lesson. – woockashek Nov 13 '16 at 23:22