2012-01-27 2 views
3

У меня есть следующая основная функция, создающая произведение коэффициентов с помощью указателей. Это лишь малая часть проекта, которая используется для создания полиномов:C++ Перезапись уже определенной переменной

#include "header.h" 
int main() 
{ 
    TermProd x = TermProd(new Coeff(4), new Coeff(8)); 
    x.print(); cout << endl; 
    x = TermProd(new Coeff(8), new Coeff(15)); 
    x.print(); cout << endl; 
    return 0; 
} 

После тестирования, перезапись, кажется, работает. Но когда я вызываю печать на x, я получаю ошибку сегментации. Я уже давно пытаюсь и смотрю на него, но я не могу понять настоящую проблему. Также мои поиски не привели к правильному направлению, поэтому я решил создать небольшой фрагмент кода, который воспроизводит ошибку.

Мой файл header.h выглядит следующим образом:

class Term{ 
public: 
    Term(){}; 
    virtual ~Term(){}; 
     virtual Term* clone() const = 0; 
    virtual void print() const = 0; 
}; 

class Coeff:public Term{ 
    int m_val; //by def: >=0 
public: 
    // Constructor 
    Coeff(); 
    Coeff(int val = 1):m_val(val) 
    // Copy constructor 
    Coeff* clone() const{return new Coeff(this->val());} 
    // Destructor 
    ~Coeff(){} 
    // Accessors 
    int val() const{return m_val;} ; 
    // Print 
    void print() const{cout << m_val; } 
}; 

class TermProd:public Term{ 
    TermPtr m_lhs, m_rhs; 
public: 
    // Constructor 
    TermProd(TermPtr lhs, TermPtr rhs):m_lhs(lhs), m_rhs(rhs){ } 
    // Copy constructor 
    TermProd* clone() const 
    { 
     return new TermProd(m_lhs->clone(), m_rhs->clone()); 
    } 
    // Destructor 
    ~TermProd(){ delete m_lhs;delete m_rhs;} 
    // Accessors 
    Term* lhs() const { return m_lhs; } 
    Term* rhs() const { return m_rhs; } 
    // Print 
    void print() const 
    { 
     cout << "("; m_lhs->print(); cout << '*'; m_rhs->print(); cout << ")"; 
    }  
}; 
+2

Прочтите это: [Правило 3] (http://stackoverflow.com/questions/4172722/what-is-the-rule-of -three) –

+0

@Michael он будет генерировать временный объект, который будет присвоен первому. – AlexTheo

ответ

3

Вы не следуете Rule of Three.
Уничтожение временных переменных, созданных во время вызовов функций, путем вызова неявного конструктора копирования delete s назначенного элемента указателя.

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

3

Вы не предоставляете конструктор копирования. У вас есть метод clone, который, по словам комментатора, является конструктором копирования, но это не так.

Try что-то вроде:

TermProd(TermProd const & other) 
    m_lhs(other.m_lhs->clone()), 
    m_rhs(other.m_rhs->clone()) 
{ 
} 

А также для других классов.

Update Как было отмечено в комментариях, вам также потребуется оператор присваивания .

TermProd & operator=(TermProd const & other) 
{ 
    if (this != &other) // Check for assignment to self. 
    { 
    delete m_lhs; 
    delete m_rhs; 
    m_lhs = other.m_lhs->clone(); 
    m_rhs = other.m_rhs->clone(); 
    } 
    return *this; 
} 
+1

И тогда 'clone()' можно упростить, чтобы возвращать новый TermProd (* this); '. –

+0

Я не думаю, что здесь проблема. Неверный код - это назначение, которое вызывается 'operator =', а не конструктор копирования. – JaredPar

+0

Вам также понадобится оператор копирования-назначения, 'TermProd & operator = (TermProd const &);' –

6

Здесь следует отметить, что вы не перезаписать переменную x но вместо присвоения ему. Это будет вызывать по умолчанию operator= для вашего типа, который грубо вызывает следующий код, который будет выполняться

  1. Конструктор TermProd::TermProd(TermPtr, TermPtr) выполняется
  2. Значения m_lhs и m_rhs копируется в значение x
  3. деструктора стоимость, созданная на шаге 1 теперь управляют и m_lhs и m_rhs удаляются

в этой точке йо У вас настоящая проблема. После шага 2 оба значения x и временное значение, созданное на шаге # 1, имеют одинаковые значения m_lhs и m_rhs. Деструктор на шаге 3 удаляет их еще x еще есть ссылка на них, которые в настоящее время эффективно указывая на мертвой памяти

Для того, чтобы решить эту проблему, вам нужно будет добавить свой собственный operator=, который правильно обрабатывает семантику присваивания.Например

TermProd& operator=(const TermProd& other) { 
    if (&other != this) { 
    delete m_lhs; 
    delete m_rhs; 

    m_lhs = other.m_lhs->clone(); 
    m_rhs = other.m_rhs->clone(); 
    } 
    return *this; 
}; 

Для того, чтобы быть правильными для всех сценариев вы также должны добавить надлежащий конструктор копирования

TermProd::TermProd(const TermProd& other) : 
    m_lhs(other.m_lhs->clone()), 
    m_rhs(other.m_rhs->clone()) 
{ 

} 

Действительно, хотя сделать это очень просто, вы должны рассмотреть возможность использования std::shared_ptr<Term> в качестве значения для TermPtr. Это указатель, который сделает совместную работу без всех вышеупомянутых накладных расходов

+0

Ваш оператор назначения должен проверить, назначаете ли вы себя, и в этом случае обычно ничего не делают. – Lindydancer

+0

@ Lindydancer oops, good catch. обновленный – JaredPar