How's the Copy & Swap idiom really supposed to work, seriously! My code fails

Viewed 87

I have been hearing about this, very advertised way of implementing an assign operator=() in C++.

EDIT: MVP(s),

Version 2:

a.out: main.cpp:52: int main(): Assertion `ptr == a.data()' failed.                                                        
Aborted (core dumped) 

Here's where it seems to fail,

A copy operator=() without Copy & Swap Idiom:

Data& operator=(const Data& rhs)
{
    if(this != &data)
    {
        _size = rhs.size();
        delete[] local_data;
        local_data = new char[_size];
        std::copy(rhs.data(), rhs.data() + rhs.size(), local_data);
    }
    return *this;
}
  1. Main.cpp with above (non-swap idiom) implementation called,

    int main(){
       Data a("Some data");
       auto ptr = a.data(); // Obtains a pointer to the original location
       a = Data("New data"); // When a is modified, this modification should effect the 'ptr'
       assert(ptr == a.data()); // Succeeds
       return 0;
     }
    

Same as above, except with Copy & Swap Idiom:

void swap(Data& rhs) noexcept
{
    std::swap(_size, rhs.size());
    std::swap(local_data, rhs.data());
}

Data& operator=(const Data& rhs)
{
    Data tmp(data);
    swap(data);
    return *this;
}

2: With swap idiom implementation:

int main(){
    Data a("Some data");
    auto ptr = a.data(); // Obtains a pointer to the original location
    a = Data("New data"); // When a is modified, this modification should effect the 'ptr' 
    assert(ptr == a.data()); // Fail
    return 0;
}

I observe, that there's clean up. But, is the clean and easy implementation of Copy & Swap Idiom supposed to fail here. And, I know that some runtime overhead does occur. But, generally, the Copy & Swap idiom seems a lot cleaner and better.

EDIT: I am aware of the self-assignment problem, and that this is supposed to help with that. Self-assignment is exceedingly rare in most programs, so an explicit check adds a small cost to every self-assignment even though the check is almost always false.

Sutter & Alexandrescu shows an assignment operator written in terms of a swap member. This is safe against self-assignment, exception-safe, and re-uses the copy constructor.

Full code for version 2:

#include <iostream>
#include <cassert>
using namespace std;

class Data 
{
private:
    char* local_data;
    int _size = 0;
    
    inline int length(const char* str)
    {
        int n = 0;
        while(str[++n] != '\0');
        return n;
    }
    
public:
    Data() {
        local_data = new char[_size];
    }
    
    Data(const char* cdata) : _size { length(cdata) }{
        local_data = new char[_size];
        std::copy(cdata, cdata + _size, local_data);
    }
    
    int size() const { return _size; }
    const char* data() const { return local_data; }
    
    void swap(Data& rhs) noexcept
    {
        std::swap(_size, rhs._size);
        std::swap(local_data, rhs.local_data);
    }
    
    Data& operator=(const Data& data)
    {
        Data tmp(data);
        swap(tmp);
        return *this;
    }
};



int main()
{
    Data a("Some data");
    auto ptr = a.data(); // Obtains a pointer to the original location
    a = Data("New data"); // When a is modified, this modification should effect the 'ptr' (char*)
    assert(ptr == a.data()); // Fails
    return 0;
}
1 Answers

The main problem is that you seem to misunderstand how modifications to a.local_data are propagated (or not, as is what happens) to ptr.

Lets draw this up, statement by statement:

  1. First we have

    Data a("Some data");
    

    Which gives us something like

    +--------------+     +-------------+
    | a.local_data | --> | "Some data" |
    +--------------+     +-------------+
    
  2. Then we "copy" the pointer into the variable ptr:

    auto ptr = a.data();
    

    Which gives us something like

    +--------------+
    | a.local_data | --\
    +--------------+    \     +-------------+
                         >--> | "Some data" |
    +-----+             /     +-------------+
    | ptr | -----------/
    +-----+
    
  3. With the next statement two things happen:

    1. First a new temporary object is created with Data("New data"):

      +--------------+
      | a.local_data | --\
      +--------------+    \     +-------------+
                           >--> | "Some data" |
      +-----+             /     +-------------+
      | ptr | -----------/
      +-----+
      
      +------------------+     +------------+
      | temporary_object | --> | "New data" |
      +------------------+     +------------+
      
    2. Then in the first version where you delete[] and new[] in the copy-assignment operator this happens:

      +--------------+     +------------+
      | a.local_data | --> | "New data" |
      +--------------+     +------------+
      
      +-----+
      | ptr | --> ???
      +-----+
      
      +------------------+     +------------+
      | temporary_object | --> | "New data" |
      +------------------+     +------------+
      

      The temporary object is then destructed, leaving you with:

      +--------------+     +------------+
      | a.local_data | --> | "New data" |
      +--------------+     +------------+
      
      +-----+
      | ptr | --> ???
      +-----+
      

      Here the value of pointer ptr is no longer valid!

    3. In the copy-and-swap version something else happens:

      +--------------+     +------------+
      | a.local_data | --> | "New data" |
      +--------------+     +------------+
      
      +-----+
      | ptr | ---------------\
      +-----+                 \     +-------------+
                               >--> | "Some data" |
      +------------------+    /     +-------------+
      | temporary_object | --/
      +------------------+
      

      Then the temporary object is destructed leaving you again with:

      +--------------+     +------------+
      | a.local_data | --> | "New data" |
      +--------------+     +------------+
      
      +-----+
      | ptr | --> ???
      +-----+
      

      As before the value of ptr is no longer valid.

However the important thing is the disconnect between ptr and a.local_data. You can modify one, but the other will not be modified. Which of course leads to the assert to fail.

That in your example it works for the first case is a coincidence, as the memory allocator seems to be reusing the memory passed to delete[]. This is of course nothing that can be guaranteed to happen always or even repatedly. As a possible way to break the first "working" example, try creating a new object with a longer string, as in

a = Data("Much longer string");
Related