Должен ли я удалить указатель источника в функции назначения оператора?

Я написал следующий демонстрационный код, чтобы изучить конструктор копирования и оператор присваивания. Однако есть небольшая путаница. Мне сказали удалить указатели в операторе присваивания и выделить новый адрес для data. Однако я могу только заставить свой код работать, удаляя эту строку. Я взял это в качестве ссылки, но она показывает только пример int, а не int*. Как я могу решить эту проблему?

#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;
}

c++ assignment-operator copy-constructor
person einverne    schedule 05.01.2016    source источник
comment
int* p_old = rhs.data; должно быть int* p_old = data;   -  person πάντα ῥεῖ    schedule 05.01.2016
comment
К вашему сведению, безопасно вызывать delete для указателя NULL ( т. е. вам не нужно делать if (data) перед вызовом delete).   -  person James Adkison    schedule 05.01.2016
comment
И data = new int(*p_old); должно быть data = new int(*rhs.data);   -  person πάντα ῥεῖ    schedule 05.01.2016
comment
Если у вас есть задание a = b, вы ожидаете, что b будет изменено?   -  person Some programmer dude    schedule 05.01.2016
comment
Я бы скромно предложил использовать умные указатели и nullptr...   -  person gilgamash    schedule 05.01.2016
comment
@JamesAdkison Спасибо за совет.   -  person einverne    schedule 05.01.2016
comment
@gilgamash Сейчас я изучаю boost.shared_ptr. И я неправильно понимаю некоторые ключевые моменты. И спасибо за ваш совет. Позже я изучу возможности C++ 11.   -  person einverne    schedule 05.01.2016

Ответы (2)


arrow_upward
2
arrow_downward

Вы пытаетесь удалить элемент data другого объекта, хотя вместо этого вы хотите удалить текущий член data своего собственного (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;
}
person mindriot    schedule 05.01.2016
comment
О, я сделал ошибку. Я что-то не понимаю, спасибо. - person einverne; 05.01.2016

arrow_upward
0
arrow_downward

Ваш код терпит неудачу, потому что вы дважды удаляете что-то: вы удаляете указатель в присваивании копии и в деструкторе. Удаление указателя не делает его нулевым, поэтому он передает if в вашем деструкторе. (Кстати, вам не нужно проверять указатель на нуль перед его удалением, delete все равно выполняет проверку)

Я бы рекомендовал копировать назначение, чтобы не изменять переменную rhs, так как удаление указателя data может легко привести к доступу к памяти. Я бы предпочел реализовать конструктор перемещения и присваивание, чтобы сделать это поведение явным. Я бы удалил конструктор копирования и назначение и добавил эту функцию:

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);
}

Для этого потребуется, чтобы std::move вызывался по заданию.

В качестве альтернативы вы можете реализовать конструктор копирования без кражи. Для этого потребуется глубокое копирование указателя данных (копирование содержимого вместо указателя).

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);
}
person Guillaume Racicot    schedule 05.01.2016