Это хороший код? (конструктор копирования и оператор присваивания) - PullRequest
Новая
       37

Это хороший код? (конструктор копирования и оператор присваивания)

10 голосов
/ 22 сентября 2009

По той или иной причине я вынужден предоставить и конструктор копирования, и оператор = для моего класса. Я думал, что мне не нужно operator=, если я определил копию ctor, но QList хочет один. Откладывая это в сторону, я ненавижу дублирование кода, так что с этим что-то не так? 

Fixture::Fixture(const Fixture& f) {
    *this = f;
}

Fixture& Fixture::operator=(const Fixture& f) {
    m_shape         = f.m_shape;
    m_friction      = f.m_friction;
    m_restitution   = f.m_restitution;
    m_density       = f.m_density;
    m_isSensor      = f.m_isSensor;
    return *this;
}

И просто из любопытства нет способа переключить его так, чтобы основная часть кода находилась в копии ctor, а operator= каким-то образом ее использовал? Я попробовал return Fixture(f);, но это не понравилось.

Похоже, мне нужно прояснить, что конструктор копирования и оператор присваивания были неявно отключены классом, от которого я наследую. Зачем? Потому что это абстрактный базовый класс, который не должен создаваться самостоятельно. Этот класс, однако, - это , предназначенный для самостоятельной работы.

Ответы [ 6 ]

25 голосов
/ 22 сентября 2009

Это плохо, потому что operator= больше не может полагаться на объект настройки. Вы должны сделать это наоборот, и можете использовать идиому копирования-обмена.

В случае, когда вам просто нужно скопировать все элементы, вы можете использовать неявно сгенерированный оператор присваивания.

В других случаях вам придется дополнительно что-то делать, в основном освобождая и копируя память. Вот где идиома копирования-обмена хороша. Он не только элегантен, но и обеспечивает, чтобы назначение не создавало исключений, если оно заменяет только примитивы. Давайте класс, указывающий на буфер, который вам нужно скопировать: 

Fixture::Fixture():m_data(), m_size() { }

Fixture::Fixture(const Fixture& f) {
    m_data = new item[f.size()];
    m_size = f.size();
    std::copy(f.data(), f.data() + f.size(), m_data);
}

Fixture::~Fixture() { delete[] m_data; }

// note: the parameter is already the copy we would
// need to create anyway. 
Fixture& Fixture::operator=(Fixture f) {
    this->swap(f);
    return *this;
}

// efficient swap - exchanging pointers. 
void Fixture::swap(Fixture &f) {
    using std::swap;
    swap(m_data, f.m_data);
    swap(m_size, f.m_size);
}

// keep this in Fixture's namespace. Code doing swap(a, b)
// on two Fixtures will end up calling it. 
void swap(Fixture &a, Fixture &b) {
  a.swap(b);
}

Вот так я обычно пишу оператор присваивания. Читать Хотите скорость? Передать по значению о необычной сигнатуре оператора присваивания (передать по значению).

8 голосов
/ 22 сентября 2009

Копирование ctor и назначение полностью различны - для назначения обычно требуется освободить ресурсы в объекте, который он заменяет, copy ctor работает с еще не инициализированным объектом. Поскольку здесь у вас, по-видимому, нет особых требований (не требуется «освобождение» при назначении), ваш подход в порядке. В общем, вы можете иметь вспомогательный метод «освободить все ресурсы, которые содержит объект» (который вызывается в dtor и в начале присваивания), а также часть «скопировать эти другие вещи в объект», которая достаточно близка на работу типичной копии ctor (или, в основном, в любом случае; -).

6 голосов
/ 22 сентября 2009

Вы просто делаете пошаговое копирование и назначение в своих примерах. Это не то, что вам нужно написать самостоятельно. Компилятор может генерировать неявные операции копирования и присваивания, которые делают именно это. Вам нужно написать свой собственный конструктор копирования, присваивание и / или деструктор, если сгенерированные компилятором не подходят (то есть, если вы управляете каким-либо ресурсом через указатель или что-то в этом роде)

3 голосов
/ 22 сентября 2009

Я думаю, что вы столкнетесь с проблемами, если ваш оператор станет виртуальным.

Я бы порекомендовал написать функцию (может быть статическую), которая будет выполнять копирование, а затем использовать конструктор копирования и оператор = вызвать эту функцию.

2 голосов
/ 22 сентября 2009

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

В книге Джеймса Коплина 1991 года Advanced C ++ это описано как часть "Православной канонической формы". В нем он выступает за конструктор по умолчанию, конструктор копирования, оператор присваивания и деструктор.

В общем, вы должны использовать православную каноническую форму, если:

  • Вы хотите поддержать присвоение объекта класса или передать эти объекты в качестве параметров вызова по значению в функцию, и
  • Объект содержит указатели на объекты с подсчетом ссылок, или деструктор класса выполняет delete для члена данных объекта.

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

Коплиен предлагает страницы с причинами для этого паттерна, и я не мог здесь их судить. Однако ключевой элемент, который уже был затронут, - это возможность очистить перезаписываемый объект.

1 голос
/ 22 сентября 2009

Я думаю, вы должны инициализировать переменные-члены вашего объекта, используя initializer list. Если ваши переменные имеют значение primitive-types, то это не имеет значения. В противном случае присвоение отличается от инициализации.

Вы можете сделать это с небольшим фокусом, инициализируя указатели внутри copy constructor до 0, затем вы можете безопасно вызывать delete в assignment operator: 

Fixture::Fixture(const Fixture& f) : myptr(0) {
    *this = f;
}
Fixture& Fixture::operator=(const Fixture& f) {
    // if you have a dynamic array for example, delete other wise.
    delete[] myptr;
    myptr = new int[10];
    // initialize your array from the other object here.
    ......
    return *this;
}
...