-5

I don't understand what is wrong when I overload operator + (purpose of this is to join 2 stacks in one new) ... it returns "sum" of but change values for those previous. ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

template <typename T>
classStack
{
    private:

        struct Node
        {
            T data;
            Node *next;
        } *top;

        std::size_t size;

    public:
        Stack();
        ~Stack();
        void            push(T data);
        void            pop(void);
        size_t          get_size(void);
        const Stack&    operator=(const Stack &stack_obj);
        const Stack     operator+(const Stack &stack_obj);
        void            show_all_stack(void);
};


template <typename T>
const Stack<T>  Stack<T>::operator+(const Stack &stack_obj)
{
    Stack   stack;
    Node    *tmp;

    if (!this->size && !stack_obj.size) {
        return stack;
    }
    if (!stack_obj.size)
    {
        stack.size = size;
        stack.top = top;
    }
    else if (!size)
    {
        stack.size = stack_obj.size;
        stack.top = stack_obj.top;
    }
    else
    {
        stack.size = size + stack_obj.size;
        stack.top = new Node;
        stack.top = top;    
        tmp = stack.top;
        while (tmp->next)
            tmp = tmp->next;
        tmp->next = new Node;
        tmp->next = stack_obj.top;
   }
   return stack;
 }

Default constructor

template <typename T>
Stack<T>::Stack(void): top(nullptr), size(0)
{   
}

Destructor

template <typename T>
Stack<T>::~Stack(void)
{
    Node    *next;

    if (!size)
        std::cout << "Stack is empty!\n";
    else
    {
        while (top != nullptr)
        {
            next = top->next;
            delete top;
            top = next;
        }
        top = nullptr;
    }
}

Assignment operator

template <typename T>
const Stack<T>& Stack<T>::operator=(const Stack<T> &stack_obj)
{
    Node    *tmp;
    Node    *ptr;
    Node    *last;
    Node    *new_node;

    if (&stack_obj != this)
    {
        while (top != nullptr)
        {
            tmp = top;
            top = top->next;
            delete tmp;
        }
        top = nullptr;
        size = stack_obj.size;
        ptr = stack_obj.top;
        while (ptr)
        {
            new_node = new Node;
            new_node->data = ptr->data;
            new_node->next = nullptr;
            if (!top)
            {
                top = new_node;
                last = new_node;
            }
            else
            {
                last->next = new_node;
                last = new_node;
            }
            ptr = ptr->next;
        }
    }
}
zero n
  • 3
  • 2
  • 2
    We need to know the copy semantics of `Stack` (the copy constructor, assignment operator, and destructor). The bottom line is that if `Stack` has incorrect or buggy copy semantics, returning `Stack` by value is also faulty. In addition, you should not implement functions that return `Stack` by value (such as your `operator +`) unless the copy semantics are correct. – PaulMcKenzie Nov 30 '18 at 06:11
  • `stack.top = top;` just assigned a pointer owned by `this` to another object. Now both objects are referencing the same nodes. You need to copy the nodes from `this` and `stack_obj` and insert the copies into `stack`. Side note: `stack.top = new Node;` followed by `stack.top = top;` is a leak. That new `Node` is lost. You want to make new `Node`s, but you don't want to overwrite the only pointer you have to them. – user4581301 Nov 30 '18 at 06:16
  • For more on the bug @Paul is warning about, read [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) and then read up on its pals the [Rules of Five and Zero.](https://en.cppreference.com/w/cpp/language/rule_of_three) Your goal should be to keep resource owning classes Rule of Three or Five compliant so that users of those classes can adhere to the Rule of Zero and stay as ignorant as possible. – user4581301 Nov 30 '18 at 06:21
  • Similar question asked earlier today: [Why is concatenating two linked lists by assigning pointers wrong?](https://stackoverflow.com/questions/53549520/why-is-concatenating-two-linked-lists-by-assigning-pointers-wrong) – user4581301 Nov 30 '18 at 06:22
  • No copy constructor? If not, then you're in trouble -- `operator +` and any function returning `Stack` is broken. Second, your assignment operator does not return a value when it must return one (you declared it to return a `Stack&)`. Third, move that code out of the assignment operator, and implement a copy constructor. Then once you have the copy constructor and destructor working properly, the assignment operator becomes very simple. So overall, none of your issues has to do with `operator +` directly -- the basics of your copying is broken (as mentioned in my first comment). – PaulMcKenzie Nov 30 '18 at 06:27
  • Thank you, something become more clear, i will try to make it better. – zero n Nov 30 '18 at 06:32
  • Unrelated: Take a look at the [Copy and Swap Idiom](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) for an easy-to-write and safe (but sometime unnecessarily slow) way to implement the assignment operator (`operator=`) based on the copy constructor. – user4581301 Nov 30 '18 at 06:35
  • [What are the basic rules and idioms for operator overloading?](https://stackoverflow.com/questions/4421706/what-are-the-basic-rules-and-idioms-for-operator-overloading) is a poor duplicate. While that link will really help the asker later, the Asker's real problem is with [Shallow Copy vs Deep Copy](https://stackoverflow.com/questions/184710/what-is-the-difference-between-a-deep-copy-and-a-shallow-copy) and [The Rule of Three.](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – user4581301 Nov 30 '18 at 06:37

1 Answers1

1

Before creating functions that return Stack<T> by value, or have functions that require copy semantics for Stack<T> (as in your operator +), you need to make sure that Stack<T> is safely copyable. Right now, you are lacking a copy constructor, thus your operator + would never work correctly, even if the function itself is bug-free.

You're missing this function:

Stack::Stack(const Stack<T>&)

The easiest way to implement the copy constructor is to take most of your code out of the assignment operator, and place that into the copy constructor. More or less, the code should look something like this (the code has not been compiled or tested -- it is there to illustrate the main point of this answer):

template <typename T>
Stack<T>::Stack<T>(const Stack<T> &stack_obj) : top(nullptr), size(0)
{
    Node    *tmp;
    Node    *ptr;
    Node    *last;
    Node    *new_node;

    ptr = stack_obj.top;
    while (ptr)
    {
        new_node = new Node;
        new_node->data = ptr->data;
        new_node->next = nullptr;
        if (!top)
        {
            top = new_node;
            last = new_node;
        }
        else
        {
            last->next = new_node;
            last = new_node;
        }
        ptr = ptr->next;
    }
}

You have already written a destructor that should work, so we won't get into that function, but a working destructor is required for the next step -- implementation of the assignment operator.

Now that you have a copy constructor and destructor, you can now write the assignment operator. The technique used here is the copy / swap idiom

#include <algorithm>
//...
template <typename T>
Stack<T>& Stack<T>::operator=(const Stack<T> &stack_obj)
{
    if ( &stack_obj != this )
    {
        Stack<T> temp(stack_obj);
        std::swap(top, temp.top);
        std::swap(size, temp.size);
    }
    return *this;
}

Believe it or not, this works. The code that used to be here was moved to the copy constructor, and thus the assignment operator is going to take advantage of the copy constructor.

To explain briefly, the code creates a temporary Stack<T> object from the passed-in stack_obj (this is why you need a working copy constructor). Then all that is done is to swap out the this members with the temporary members. Last, the temp dies off with the old data (this is why the destructor needs to be working correctly).

Note that you need to swap all of the members, so if you add more members to the Stack<T> class, you need to swap them in the assignment operator.

Once you have the basic 3 functions (copy, assign, destroy) written correctly, then and only then should you write functions that return Stack<T> by value, or write functions that take a Stack<T> by value.

You may have other issues with operator + that lie outside the scope of what this answer is presenting to you, but the gist of it is that your class requires that it has correct copy semantics before implementing +.

PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45