C++ dynamic memory

The problem this: at the end of this function, the members of the element at "tasks[taskCount]" like name, due date, etc, are indeed what was passed into this function, but after returning to this function's caller, all those values become garbage, except taskcount, which is not dynamic. This function is defined within the scope of class, "Tasklist"

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

Here is the brief definition for class "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) + 开发者_JS百科1];

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

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

I'm pretty sure what is happening is that this function is disposing of its locally declared variable "task" after returning to the caller, which invokes task's destructor thereby deallocating the memory that the "tasks" array was referencing for each of it's element's members (name, due, course).

So, how do I prevent this from happening?

So by the advice of the many helpful people on this site, I now have this in my Task class definition:

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

So this should account for one of the rule of three, right?

You are violating the rule of three, by not writing a copy constructor that provides proper deep-copy semantics to your Task type.

You should use std::string instead of char * and let C++ handle allocation for you. No need to call operator new[], strcpy(), or operator delete[] when these operations have a better interface in the form of std::string.

If you cannot use std::string, then you need to implement a copy constructor for Task that takes a const Task& as its only argument, and an assignment operator that does roughly the same thing. This constructor will then be implicitly used by your code when copying a Task object into an array or other place:

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

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

(These two members tend to have very similar implementations. Factoring out the common code is an exercise for the reader.)

Have you properely overloaded copy constructor and assignment operator?

You have to create a new copy of those strings.

You need to implement the "rule of three". That means that any time you have a constructor that creates resources and a destructor that frees them, you also need to create a copy constructor and an assignment operator that copies these resources properly too.

Right now, you don't have a copy constructor or an assignment operator, so those items are not getting copied.

I'm pretty rusty at C++, but it looks like Task is allocated on the stack, not on the heap, so when task goes out of scope, it's unallocated.

You are on the right track that the locally declared variable task is destroyed and then the memory is freed.

The problem is that this line:

tasks[taskCount] = task;

makes a copy of the Task object, probably by using the default copy constructor of the Taskclass. The default copy constructor only copies the pointer values, it does not allocate new memory like your constructor does. So you now have to objects, referring to the same memory and when the first one is destructed it frees the memory.

To remedy this:

1. Use the std::string class instead of storing strings in hand-allocated buffers. Using the std::string class is the C++ way of doing this. char* is the C way, which shouldn't be used in C++ code.
2. When dealing with dynamically allocated memory, use smart pointers such as tr1::shared_ptr that will take care of deallocation for you - deallocating when the last pointer to the memory block is released.

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

You are allocating a temporary variable on the stack which you copy by value into the array, then C++ deallocates the temporary variable since it was allocated on the stack. This means that those char[] pointers inside the object become dangling pointers - they no longer point to valid stuff.

Either use std::string instead of char[] arrays, they are copied by value instead of by pointer, or modify your tasks array to contain Task* pointers (e.g. std::vector<Task*>):

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

Edit: Or as others have pointed out, write a copy constructor and an assignment operator if you wanna copy by value a class which contains pointers. I recommend the std::string approach though, it is much simpler.

Forward: Use std::string. Please. Pretty please. Doing your own memory management is going to end in tears, I promise.

Let's annotate your addTask function

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

The interesting thing is to look at Task::operator=(const Task&), that does a bitwise copy of your Task object. Unless you explicitly specified it, it looks like this:

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

So you can see the problem -- the operator= is just copying the pointer to the char[] arrays, not the entire arrays. When the array gets destroyed, the pointer to them is not fixed. Because you are using manual memory management, you must explicitly write your own operator= and copy contstructor that does a "deep copy" versus a "shallow copy".

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  ... */
}

This is why manual memory management blows!

Your copy constructor looks fine. You need to create the Task item on the heap, not on the stack. I'm not sure where or how the tasks array is defined, but it needs to be something like this:

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

Now you need a good spot to do call a cleanup method that will do something like:

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

Note that you're not doing anything with saveText in your copy constructor. I'm not sure what this is, but you need to handle it like the other members. I put in a hack in the destructor to illustrate the problem, heh. I'd assign it to NULL in the copy constructor.