CODEWITHSUNDEEP
c++c++11
user14368465
user14368465

Reputation:

Move operator destroying original pointer

I am trying to have a pointer to a memory location. Then, if I modify the original the assigned variable who points to the same location must be also affected by the change.

But, the thing is, there are two function calls, that happen on line:

a = Data("New data");
  1. Data(const char* cdata) constructor is called.
  2. Data operator=(Data&& data) move operator is called.

EDIT: I am aware of the folloing:

but, I am trying to accomplish this without these new features. I am fully aware.

My C++ code:

class Data 
{
private:
    char* local_data;
    int _size = 0;
    int length(const char* c)
    {
        int i = 0;
        while(c[++i] != '\0');
        return i;
    }
public:
    Data() {
        local_data = new char[_size];
    }
    
    Data(const char* cdata){
        _size = length(cdata);
        local_data = new char[_size];
        memcpy(local_data, cdata, _size);
    }
    
    int size() { return _size; }
    char* data() { return local_data; }
    const char* data() const { return local_data; }
    
    Data& operator=(const Data& data){}
    Data& operator=(Data&& data){
        
        if(this == &data)
            return *this;
        
        _size = std::move(data.size());
        local_data = std::move(data.data());
        return *this;
    }
};

int main(){
    
    Data a("Some data");
    auto dptr = a.data(); // Gives a pointer to the original location
    a = Data("New data"); // Must modify both a and dptr
    assert(dptr == a.data()); // Should pass successfully, else fail
    return 0;
}

Upvotes: 1

Views: 221

Answers (2)

G. Sliepen
G. Sliepen

Reputation: 7973

There are a few issues with your class Data. First, it never calls delete[] local_data, so it will leak memory. Add a destructor:

~Data() {
    delete[] local_data;
}

Then ensure your move assignment operator works correctly. Don't use std::move(data.data()), this does not in fact set the pointer in the object that is moved from to nullptr. Also, ensure you clean up this->local_data properly. The easiest way to do this is:

Data& operator=(Data&& data){
    std::swap(local_data, data.local_data);
    std::swap(_size, data._size);
    return *this;
}

It swaps the pointers around, so there is no memory leak. After the move, the destruction of data will ensure the old this->local_data is deleted.

Your copy assignment operator is empty, you should fix that, and if you have a move assignment operator you probably also want to implement a move constructor. Finally:

assert(dptr == a.data()); // Should pass successfully, else fail

This is wrong; dptr should not be the same as a.data(), since dptr is a copy of the pointer to the data taken from before the move assignment. The value of dptr will not change unless you explicitly modify dptr.

Upvotes: 0

Jarod42
Jarod42

Reputation: 217478

It seems you want std::shared_ptr<std::string>:

class Data 
{
private:
    std::shared_ptr<std::string> local_data = std::make_shared<std::string>();
public:
    Data() = default;
    
    Data(const char* cdata) : local_data(std::make_shared<std::string>(cdata)) {}
    
    int size() const { return local_data->size(); }
    char* data() { return local_data->data(); }
    const char* data() const { return local_data->c_str(); }
    
    Data operator=(const Data& rhs) {
        if (this == &data)
            return *this;
        
        *local_data = *rhs.local_data;
        return *this;
    }
};

int main(){
    
    Data a("Some data");
    auto dptr = a.data(); // Gives a pointer to the original location
    a = Data("New data"); // Must modify both a and dptr
    assert(dptr == a.data()); // Should pass successfully
}

Upvotes: 2

Related Questions