Should I delete origin pointer in operator assignment function?

Question

I have write following demo code to learn copy constructor and assignment operator. However there is a little confuse. I was told to delete pointers in assignment operator and allocate new address to the data. However I can only make my code work delete that line. I have taken this page as a reference, but it only shows the example of int while not the int*. How can I solve this problem?

#include <iostream>
#include <string>
#include <vector>
#include <random>
#include <boost/smart_ptr.hpp>
#include <boost/make_shared.hpp>

using namespace boost;

class ClassOne
{
public:
    ClassOne():data(NULL) {}
    ClassOne(int data_param):data(NULL)
    {
        init(data_param);
        std::cout << "construct" << std::endl;
    }

    virtual ~ClassOne()
    {
        if (data) {
            delete data;
        }
        data = NULL;
    }

    ClassOne(const ClassOne& rhs){
        std::cout<< "copy " <<std::endl;
        data = NULL;
        init(*rhs.data);
    }

    ClassOne& operator = (const ClassOne& rhs){
        std::cout<< "assign " <<std::endl;
        int* p_old = rhs.data;
        data = new int(*p_old);
        //delete p_old;            // I have to delete this line to make my code work
        return *this;
    }

    void init(int data_param)
    {
        if (data) {
            delete data;
        }
        data = new int(data_param);
    }

private:
    int* data;
};

int main(int argc, const char * argv[]) {
    ClassOne c1(10);
    ClassOne c2(c1);       // call copy constructor
    ClassOne c3;
    c3 = c1;               // call assignment function
    return 0;
}

FYI, [it's safe to call `delete` on a `NULL` pointer](http://stackoverflow.com/questions/4190703/is-it-safe-to-delete-a-null-pointer) (i.e., you don't need to do `if (data)` before calling `delete`). — James Adkison, Jan 05 '16 at 13:59
And `data = new int(*p_old);` should be `data = new int(*rhs.data);` — πάντα ῥεῖ, Jan 05 '16 at 13:59
If you have the assignment `a = b`, would you expect `b` to be modified? — Some programmer dude, Jan 05 '16 at 14:00
@gilgamash I am learning boost.shared_ptr now. And I misunderstand some keypoints. And thank you for your advice. Later I will learn C++ 11 features. — einverne, Jan 05 '16 at 14:06

score 2 · Accepted Answer · answered Jan 05 '16 at 13:59

2

You are trying to delete the other object's data member, when you are meant to delete your own (this) object's current data member instead. What you probably want is this:

ClassOne& operator = (const ClassOne& rhs){
    std::cout<< "assign " <<std::endl;
    delete data;               // delete your own old data
    data = new int(*rhs.data); // clone the rhs's data
    return *this;
}

answered Jan 05 '16 at 13:59

mindriot

5,413
1
25
34

Oh, I made a mistake. I misunderstand something, thanks. – einverne Jan 05 '16 at 14:01

Guillaume Racicot · Answer 2 · 2016-01-05T14:17:48.177

Your code fail because you are double deleting something: you delete the pointer in the copy assignment and in the destructor. Deleting a pointer don't make it null, that's why it passes the if in your destructor. (By the way, you don't need to check the pointer to be null before deleting it, delete does the check anyway)

I would recommend copy assignment to not alter the rhs variable, as deleting the data pointer can easily lead to memory access. I would rather implement a move constructor and assignment to make this behaviour explicit. I would remove copy constructor and assignment and add these function:

ClassOne(const ClassOne&) = delete;
ClassOne& operator=(const ClassOne&) = delete;

ClassOne(ClassOne&& rhs) {
    std::swap(data, rhs.data);
}

ClassOne& operator=(ClassOne&& rhs) {
    std::swap(data, rhs.data);
}

That would require std::move to be call on assignment.

Alternatively, you can implement a non-stealing copy constructor. That would require that youto deep copy the data pointer (copying the content instead of the pointer)

ClassOne(const ClassOne& rhs) {
    if (!data) {
        data = new int;
    }
    *data = *(rhs.data);
}

ClassOne& operator=(const ClassOne& rhs) {
    if (!data) {
        data = new int;
    }
    *data = *(rhs.data);
}

Should I delete origin pointer in operator assignment function?

2 Answers2