C++: указатель освобождение не было выделен

Question

Я очистками игрушечной программы, которую я написал для класса с помощью Xcode 5. Части задания было получить знакомство с динамическим распределением памяти (что означает, что сусла использовать new и delete несмотря тот факт, что он действительно будет работать без него). Я получаю:

HeapOfStudents(67683,0x7fff769a6310) malloc: *** error for object 0x100300060: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug

ошибка происходит:

Student::~Student() { 
    delete firstName; // <- on this line 
    delete lastName; 
    delete address; 
    delete birthDate; 
    delete gradDate; 
    delete gpa; 
    delete crHours; 
} 

, который называют ...

int main() { 

    string line = ""; 
    vector<Student> students; 


    // 
    //Create new students from file 
    // 
    ifstream data_file ("heapData"); 

    if (data_file.is_open()) { 

     while(getline(data_file, line)) 
      students.push_back(*new Student(line)); <- inside this call here 

     data_file.close(); 
    } 

    else return 0; 

Полный Student класс ниже.

Важно отметить, что я заметил, что в то время как каждый line получает правильно толкнул к вектору, когда следующий Student получает толкаемом, предыдущий Studentвнутри вектор повреждается. Я вижу ошибку malloc всегда на третьей итерации цикла (файл имеет 50 строк).

Программа работает без исключения, если я меняю vector<Student> на vector<Student *> и student.push_back(*new Student(line)) на student.push_back(new Student(line)).

Мой вопрос: Может кто-нибудь объяснить, что на нижнем уровне происходит внутри этого цикла, что создает эту ошибку?

Я думаю, что *new Student(line) использует тот же указатель каждый раз, так в каждой итерации получает данные повреждены, так как он освобожден (хотя это объяснение вызывает вопросы), в то время как new Student возвращает новый указатель каждый раз, сохраняя данные, был передан students. Я думаю, что, возможно, мне нужно написать конструктор копий ... Это только моя догадка.

Это Student класс. Address и Date также являются обычными классами, которые довольно похожи. Я не включил их, потому что они, похоже, не являются частью проблемы.

//Constructors 
Student::Student() { 

    firstName = new string("Testy"); 
    lastName = new string("Testerson"); 
    address = new Address("000 Street St", "", "Citytown", "State", "00000"); 
    birthDate = new Date("01/02/9876"); 
    gradDate = new Date("01/02/6789"); 
    gpa = new string("10.0"); 
    crHours = new string("300"); 

} 

Student::Student(string FN, string LN, string L1, string L2, string C, string S, string Z, string BD, string GD, string GPA, string Hr) { 

    firstName = new string(FN); 
    lastName = new string(LN); 
    address = new Address(L1, L2, C, S, Z); 
    birthDate = new Date(BD); 
    gradDate = new Date(GD); 
    gpa = new string(GPA); 
    crHours = new string(Hr); 

} 

Student::Student(string line) { 
    set(line); 
} 

//Destructors 
Student::~Student() { 
    delete firstName; 
    delete lastName; 
    delete address; 
    delete birthDate; 
    delete gradDate; 
    delete gpa; 
    delete crHours; 
} 

//Member Functions 
void Student::set(string line) { 

    firstName = new string(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    lastName = new string(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    address = new Address(); 

    address->setLine1(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    address->setLine2(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    address->setCity(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    address->setState(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    address->setZip(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    birthDate = new Date(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    gradDate = new Date(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    gpa = new string(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

    crHours = new string(line.substr(0, line.find_first_of(","))); 
    line = line.substr(line.find_first_of(",") + 1, string::npos); 

} 

void Student::printReport() { 

    cout << *lastName << ", " << *firstName << ", " << address->getAddress() 
    << ", " << birthDate->getDate() << ", " << gradDate->getDate() 
    << ", " << *gpa << ", " << *crHours << endl; 

} 

void Student::printName() { 
    cout << *lastName << ", " << *firstName << endl; 
} 

string Student::getName() { 
    return string(*lastName + ", " + *firstName); 

EDIT Вот копировальную конструкторы и операторы присваивания, которые я написал для класса студент должен кто-то быть заинтересован в том, чтобы/критикуя их:

Student::Student(const Student & student){ 

    firstName = new string(*student.firstName); 
    lastName = new string(*student.lastName); 
    address = new Address(*student.address); 
    birthDate = new Date(*student.birthDate); 
    gradDate = new Date(*student.gradDate); 
    gpa = new string(*student.gpa); 
    crHours = new string(*student.crHours); 

} 

Student& Student::operator=(const Student & student) { 
    return *new Student(student); 
} 

Answer 1

Похоже, вы не следуете Rule of Three, так что плохие вещи произойдут, если вы скопируете объект Student. В частности, оба будут содержать указатели на те же объекты, которые оба попытаются удалить - так что вы попытаетесь удалить то, что уже удалило другое.

Самый простой способ, чтобы дать действительные семантику копирования класса является запретить копирование, удалив конструктор копирования и копирования назначения оператора:

class Student { 
    Student(Student const &) = delete; 
    void operator=(Student const &) = delete; 

    // rest of class... 
}; 

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

Кроме того, это приводит к утечке памяти:

students.push_back(*new Student(line)); 

путем динамического создания Student, копирование его в вектор, и отбрасывания единственный указатель на динамический объект. Если вы храните объекты, а затем нажать на копию временного:

students.push_back(Student(line)); 

В качестве альтернативы, вы можете изменить тип контейнера для vector<Student*>, и не забудьте удалить каждый объект при удалении его указатель. (Это разумная вещь, как упражнение в стрельбе по указанию, как вы говорите, но не делайте этого в любой программе, которую вы хотите сохранить.)

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

Answer 2

Может случиться так, что вы не хотите Student которые нужно скопировать, но вы все же хотите поместить их в vector. В C++ 11 она может быть решена, просто добавив конструктор перемещения:

class Student { 
public: 
    Student() { ...} 
    ~Student() {...} 
    Student(const Student&) = delete; // optional: will be deleted implicitly 

    Student(Student&& other) { 
     firstName = other.firstName; 
     other.firstName = nullptr; 
     // ... 
    } 
}; 

Таким образом, конструктор копирования и оператор присваивания будет неявно удалено, так что вы не будете в состоянии сделать копии. Но vector и другие контейнеры будут использовать конструктор перемещения просто отлично. Это, конечно, создает определенные ограничения на использование Student.

vector<Student> students; 
students.push_back(Student()); // OK, student moves 
Student s; 
students.push_back(s); // Error: copy constructor for Student is deleted 

Answer 3

Одна из проблем, у вас есть то, что вы оставляете вектор Student объектов вместо вектора указателей на динамически распределяемой объекты (типа Student* поэтому).

Просто замените переменную students на std::vector<Student*> students;. С этого момента вы можете просто указать push_back указатель, созданный new.

Дело в том, что vector уже имеет дело с распределением памяти, поэтому линия, делающая push (выделенный вами в коде), превращает копию Студента в позицию в векторе. После этого указатель на динамически выделенный объект станет недоступным.

Как указал Бармар, наличие вектора указателя также имеет преимущество в том, что он может нажимать указатели подклассов Student. Простой пример на том, что:

class PhDStudent : public Student { ... } 

students.push_back(new PhDStudent(...)); 

Кроме того, есть много других настроек вы должны рассмотреть в своем классе:

• Ваш конструктор принимает string параметров по стоимости, что означает, что они глубоко копируются от их происхождения. Использование const string& здесь предпочтительнее, чтобы избежать ненужных копий.

• Как уже указывалось в некоторых других ответах (Майк Сеймур, cdhowie, Антон Савин и тот, кто собирается это указать), вы должны следовать за Rule of Three. Если вы хотите, чтобы ваш класс был гибким, вы также должны реализовать конструктор копирования и оператор присваивания копии. Если вы используете C++ 11, вы можете воспользоваться конструктором перемещения и назначением перемещения, чтобы уменьшить количество распределений при перемещении этих объектов.

Answer 4

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

Вам необходимо следить за rule of three: если вам нужен пользовательский деструктор, оператор присваивания или конструктор копирования, вам нужны все они.

Добавьте следующие открытые члены к классу:

Student(Student const &); 
Student & operator=(Student const &); 

И определим их следующим образом:

Student::Student(Student const & other) 
{ 
    firstName = new string(other.firstName); 
    // And so on, for each pointer member. 
} 

Student & Student::operator=(Student const & other) 
{ 
    *firstName = other.firstName; 
    // And so on, for each pointer member. 

    return *this; 
} 

Обратите внимание, что все это можно избежать, используя string firstName; вместо string * firstName; - в в этом случае деструктор по умолчанию, конструктор копирования и оператор присваивания будут делать правильные вещи, и вам не нужно будет определять их.

Кроме того, имейте в виду, что у ваших классов Address и Date может возникнуть проблема! Вы должны делать эти виды гимнастики каждый раз, когда вы используете сырые указатели в качестве членов класса.