1

I have a class called Stack. I've implemented the copy constructor but for the copy assignment constructor I just have it call the copy constructor since the logic is the same. But for some reason it just returns a default constructed object. I don't understand why.

The copy constructor:

Stack::Stack( const Stack& s )
{
    if ( s.Empty() )
    {
        this->entry = nullptr;
    }
    else
    {
        this->entry = new Entry();
        Entry* i = this->entry;
        for ( Entry* p = s.entry; p != nullptr; p = p->next )
        {
            i->number = p->number;
            i->next = p->next == nullptr ? nullptr : new Entry();
            i = i->next;
        }
    }
}

The copy assignment constructor:

Stack& Stack::operator=( const Stack& s )
{
    return Stack( s );
}

The calling code:

Stack s;
s.Push( 5 );
s.Push( 3 );

Stack s2;
s2 = s; //s2 just ends up defaulted constructed instead of copied from s

If I replace the lines:

Stack s2;
s2 = s;

with:

Stack s2(s);

everything works just fine. What am I doing wrong here?

EDIT --

So the lesson here is to implement the assignment ctor and leverage that in the copy ctor, not the other way around as I had done.

Legion
  • 3,922
  • 8
  • 51
  • 95
  • *What am I doing wrong here?* -- Not implementing the assignment operator in a much simpler and less error prone way: Is there a destructor for the `Stack` class? – PaulMcKenzie Apr 27 '20 at 20:16
  • `Stack& Stack::operator=(Stack s) { swap(*this, s); return *this; }` – Eljay Apr 27 '20 at 20:17
  • @PaulMcKenzie Yes – Legion Apr 27 '20 at 20:17
  • The assignment operator must assign the passed object to `this` object and then return a reference to `this` object. You don't do that. You just create a temporary copy of the passed object and then return a reference to that temporary object, which is already destroyed when the function returns the then dangling reference. The compiler should warn you about that bug. If it does not, then raise the warning level! – Werner Henze Apr 27 '20 at 20:17
  • The best way forward I see is to swap the dependencies around. Implement the logic in the copy assignment operator and then implement the copy constructor as `*this = s;` – Louis Cloete Apr 27 '20 at 20:23
  • It seems you are using MSVC with its poor extensions, use flag: `/permissive-`, then your code will not compile, because temporary value cannot be bound to lvalue reference. – rafix07 Apr 27 '20 at 20:28
  • @Legion You can simply keep the copy constructor as-is, and have the assignment operator do swaps. – PaulMcKenzie Apr 27 '20 at 21:13

4 Answers4

3

The behaviour of your assignment operator is undefined.

You are returning a dangling reference.

The idiomatic way of writing the assignment operator is std::move the instance of the object passed (s in your case, which you should pass by value) to self, and return *this as a reference.

Bathsheba
  • 231,907
  • 34
  • 361
  • 483
3

Your assignment operator is incorrect. It is returning a reference to a temporary object. It should return *this instead:

Stack& Stack::operator=( const Stack& s )
{
   // copy s to members
   return *this;
}

If you want to avoid implementing things twice its usually easier to implement the assignment operator and use that in the copy constructor:

Stack::Stack( const Stack& s )
:Stack()
{
  *this = s;
}

Stack& Stack::operator=( const Stack& s )
{
    if (&s == this) return *this;
    // TODO: free current members?
    if ( s.Empty() )
    {
        this->entry = nullptr;
    }
    else
    {
        this->entry = new Entry();
        Entry* i = this->entry;
        for ( Entry* p = s.entry; p != nullptr; p = p->next )
        {
            i->number = p->number;
            i->next = p->next == nullptr ? nullptr : new Entry();
            i = i->next;
        }
    }
    return *this;
}
Alan Birtles
  • 32,622
  • 4
  • 31
  • 60
  • Thanks! I had things backwards, implemented the copy instead of assignment. Moving the implementation to the assignment constructor and then using `*this = s;` in the copy constructor worked perfect! – Legion Apr 27 '20 at 20:22
  • @Legion there is an excellent strategy called [Copy and Swap](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) that uses the copy constructor to implement the assignment operator. Which you use often depends on the sorts of exception safety guarantees and performance you need. – user4581301 Apr 27 '20 at 20:40
2

When you do

Stack& Stack::operator=( const Stack& s )
{
    return Stack( s );
}

Stack(s) creates an object with a scope limited to the current function. You then return a reference to this local object.

So, when doing x = y, x is not modified to become a copy of y and a reference to an object that is out of scope is returned, then discarded.

The job of the Stack& Stack::operator=( const Stack& s ) is not to return a copy. Its job is to modify x (called this) so that it becomes equal to y (called s).

So, the proper way would look something like:

Stack& Stack::operator=( const Stack& s )
{
    this->entry = new Entry();
    // then all the code to make `this` equal to `s`
}
Jeffrey
  • 11,063
  • 1
  • 21
  • 42
1

Since you have a working copy constructor and working destructor for Stack, you can simply use copy / swap to implement the assignment operator:

Stack& Stack::operator=( const Stack& s )
{
    if ( this != &s)
    {
       Stack temp(s);
       std::swap(temp.entry, entry);
    }
    return *this;
}

If you have other member variables, you will need to swap those also.

This works by creating a temporary, and simply swapping out the members of the current object (this) with the copy's members. Then the copy dies off with the old data.

PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • This. But just in case (to cover the case where the type of `entry` has a `swap` overload) do `using std::swap; swap(temp.entry, entry);` – drRobertz Aug 17 '20 at 10:36