C ++ динамическая память - PullRequest
Новая
       24

C ++ динамическая память

2 голосов
/ 24 июля 2011

Проблема в следующем: в конце этой функции элементы элемента «tasks [taskCount]», такие как имя, срок выполнения и т. Д., Действительно являются тем, что было передано в эту функцию, но после возврата к вызывающей функции этой функции.все эти значения становятся мусором, кроме taskcount, который не является динамическим.Эта функция определена в области видимости класса «Tasklist» 

void addTask(char name[],char course[],char dueDate[]){
    taskCount++;
    Task task(taskCount, name, course, dueDate);
    tasks[taskCount] = task;
}

Вот краткое определение класса «Task»: 

class Task
{
private:
    int number;
    char* name;
    char* dueDate;
    char* course;
public:
    Task(){
        name = new char[TEXT_SIZE + 1];
        dueDate = new char[TEXT_SIZE + 1];
        course = new char[TEXT_SIZE + 1];
        saveText = new char[(TEXT_SIZE * 3) + 1];
    };

    Task(int num, char n[], char c[], char d[]){
        number = num;
        name = new char[strlen(n) + 1];
        dueDate = new char[strlen(d) + 1];
        course = new char[strlen(c) + 1];

        strcpy(name, n);
        strcpy(dueDate, d);
        strcpy(course, c);
    };

    ~Task(){
        delete [] name;
        delete [] dueDate;
        delete [] course;
        delete [] saveText;
    }
};

Я почти уверен, что происходитзаключается в том, что эта функция освобождает свою локально объявленную переменную «task» после возврата к вызывающей стороне, которая вызывает деструктор задачи, освобождая тем самым память, на которую ссылался массив «tasks» для каждого из элементов своего элемента (name, due, course).

Итак, как я могу предотвратить это?

Итак, по совету многих полезных людей на этом сайте, теперь у меня есть это в моем определении класса Задачи: 

Task(const Task& t){
    name = new char[TEXT_SIZE + 1];
    dueDate = new char[TEXT_SIZE + 1];
    course = new char[TEXT_SIZE + 1];
    saveText = new char[(TEXT_SIZE * 3) + 1];

    number = t.number;
    strcpy(name, t.name);
    strcpy(dueDate, t.dueDate);
    strcpy(course, t.course);
    strcpy(saveText, t.saveText);
}

Так что это должно учитывать одно из трех правил, верно?

Ответы [ 9 ]

6 голосов
/ 25 июля 2011

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

5 голосов
/ 25 июля 2011

Вы должны использовать std::string вместо char * и позволить C ++ обрабатывать выделение для вас.Нет необходимости вызывать operator new[], strcpy() или operator delete[], когда эти операции имеют лучший интерфейс в виде std::string.

Если вы не можете использовать std::string, то вам необходимо реализоватьконструктор копирования для Task, который принимает const Task& в качестве единственного аргумента, и оператор присваивания, который делает примерно то же самое.Этот конструктор будет затем неявно использоваться вашим кодом при копировании объекта Task в массив или другое место: 

Task::Task(const Task& t) {
    ...
}

Task& Task::operator =(const Task& t) {
    if (this != &t) {
        ...
    }
    return *this;
}

(Эти два члена, как правило, имеют очень похожие реализации. Факторизация общего кодаупражнение для читателя.)

5 голосов
/ 25 июля 2011

Был ли у вас перегружен конструктор копирования и оператор присваивания?

Вы должны создать новую копию этих строк.

2 голосов
/ 25 июля 2011

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

Сейчас у вас нет конструктора копирования или оператора присваивания, поэтому эти элементы не копируются.

1 голос
/ 25 июля 2011

Я довольно ржавый в C ++, но похоже, что Task размещен в стеке, а не в куче, поэтому, когда task выходит из области видимости, он не выделяется.

0 голосов
/ 25 июля 2011

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

Task* tasks[100];

void addTask(char name[],char course[],char dueDate[]){
    Task* task = new Task(taskCount, name, course, dueDate);
    tasks[taskCount] = task;
    taskCount++;
}

 ~Task(){
    delete name;
    delete dueDate;
    delete course;
    if (saveText != NULL){
        delete saveText;
    }
}

Теперь вам нужно хорошее место для вызова метода очистки, который будет выполнять что-то вроде

void cleanupTasks() {
    for (int i = 0; i < taskCount; i++){
        delete tasks[i];
    }
}

Обратите внимание, что вы ничего не делаете с saveText в конструкторе копирования.Я не уверен, что это такое, но вы должны справиться с этим, как другие участники.Я взломал деструктор, чтобы проиллюстрировать проблему, хе.Я бы назначил его в NULL в конструкторе копирования.

0 голосов
/ 25 июля 2011

Вперед: используйте std :: string. Пожалуйста. Довольно пожалуйста. Я обещаю, что управление собственной памятью закончится слезами.

Давайте аннотируем вашу функцию addTask 

void addTask(char name[],char course[],char dueDate[])
{
    taskCount++;
    Task task(taskCount, name, course, dueDate); <--- Object 'task' created on the stack
    tasks[taskCount] = task;                     <--- Task::operator=(const Task&)
}                                                <--- Ojbect 'task' destroyed

Интересно посмотреть на Task :: operator = (const Task &), который делает побитовую копию вашего объекта Task. Если вы не указали это явно, это выглядит так: 

class Task {
    char *name, *course, *dueDate;
    /* other stuff */

    /* Provided automatically by the compiler if you don't specify it */
    Task(const Task& rsh) {
        this->name = rhs.name;
        this->course = rhs.course;
        this->dueDate = rhs.dueDate;
    }

    Task& operator=(const Task& rhs) {
        this->name = rhs.name;
        this->course = rhs.course;
        this->dueDate = rhs.dueDate;
    }
};

Итак, вы можете увидеть проблему - оператор = просто копирует указатель на массивы char [], а не на все массивы. Когда массив уничтожается, указатель на них не фиксируется. Поскольку вы используете ручное управление памятью, вы должны явно написать свой собственный оператор = и скопировать contstructor, который выполняет «глубокую копию» вместо «мелкой копии». 

Task::Task(const Task& rhs) {
    this->number = rhs.number;
    this->name = new char[ strlen(rhs.name) ];
    strcopy(this->name, rhs.name);
    /* ... for other char* in the class  ... */
}

Task::operator=(const Task& rhs) {
    this->number = rhs.number;
    if ( NULL != name ) { // note, if you have a default ctor, i.e. Task::Task(), make sure it initializes char*s to NULL
        delete[] name; // otherwise you will crash when you delete unallocated memory
    }
    this->name = new char[ strlen(rhs.name) ];
    strcopy(this->name, rhs.name);

    /* ... for other char* in the class  ... */
}

Вот почему ручное управление памятью дует!

0 голосов
/ 25 июля 2011 
Task task(taskCount, name, course, dueDate);
    tasks[taskCount] = task;

Вы размещаете временную переменную в стеке, которую вы копируете по значению в массив, затем C ++ освобождает временную переменную, поскольку она была размещена в стеке.Это означает, что эти указатели char [] внутри объекта становятся висячими указателями - они больше не указывают на допустимый материал.

Либо используйте std :: string вместо массивов char [], они копируются по значению, а не поуказатель или измените массив задач так, чтобы он содержал указатели Task * (например, std::vector<Task*>): 

    tasks[taskCount] = new Task(taskCount, name, course, dueDate);

Редактировать: Или, как указывали другие, напишите конструктор копирования и оператор присваивания, если вы хотите скопироватьзначение класса, который содержит указатели.Я рекомендую подход std :: string, хотя он намного проще.

0 голосов
/ 25 июля 2011

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

Проблема в том, что эта строка: 

tasks[taskCount] = task;

делаеткопия объекта Task, возможно, с использованием конструктора копирования по умолчанию класса Task.Конструктор копирования по умолчанию копирует только значения указателя, он не выделяет новую память, как это делает ваш конструктор.Таким образом, теперь вам нужно обращаться к объектам, ссылаясь на одну и ту же память, и когда первая разрушается, она освобождает память.

Чтобы исправить это:

  1. Вместо этого используйте класс std::stringхранения строк в выделенных вручную буферах.Использование класса std::string - это способ C ++.char* - это способ C, который не должен использоваться в коде C ++.
  2. При работе с динамически выделяемой памятью используйте умные указатели, такие как tr1::shared_ptr, которые позаботятся о вашем освобождении - освобождение припоследний указатель на блок памяти освобождается.
...