-2

If we had the following simple double linked list:

class Node{
    //...

    Node* next;
    Node* previous;
    int data;
};

class A{
    //...
    A(const A&);
    ~A(); //goes through list and deletes each node
    A& operator=(const A&);

    Node* begin;
    Node* end;
    int id;
};

A::A(const A& copyThis)
{
    //creates a new list that represnets the same data as copyThis's list
    //but is not the same list
    //Example for clarification:
    //if copyThis represented a list of nodes each containing the values 1-10 respectively
    //this constructor would make a new list of nodes each containing the values 1-10
    //and make the invoking object's members point to the beginning and end of it
    //assume this does not create any memory leaks
}

A& A::operator=(const A& a) //this causes a memory leak
{
    A* aCopy = new A(a); //now 'a' and 'this' should represent the same
    //values but point to different lists
    this->begin = aCopy->begin;
    this->end = aCopy->end;
    this->id = aCopy->id;

    //code here that deletes list that invoking object used to point to
    //assume it does correctly and without memory leak

    return *this;
}

Lets say we have this function:

void foo()
{
    A a1(); 
    //put some data into a1
    A a2();
    {a2=a1;}
}

My question is why this causes a memory leak since a2 should represent the same list of Nodes that aCopy does inside of operator= and when a2 goes out of scope it frees the memory allocated for each Node correctly.

EDIT: Okay So I just realized minutes after posting this question that maybe creating aCopy with new allocates memory for more than just the Nodes it represents and that memory is never freed. Is this correct?

EDIT: I am not asking for a better or correct way to write this code, I am ONLY asking for an explanation as to why this causes a memory leak.

MikeRizzle
  • 84
  • 7
  • why not using smart pointers? –  Nov 19 '15 at 21:27
  • 2
    Why create `aCopy` at all? – Fred Larson Nov 19 '15 at 21:29
  • @Nostradamus I just wanted to understand what was causing the leak in this situation, I know that there are many other ways to do this effectively without memory leaks. At the time, it just seemed to me that this code should not cause a memory leak. – MikeRizzle Nov 19 '15 at 21:30
  • 1
    thats right you are allocating a new memory space that you dont delete –  Nov 19 '15 at 21:30
  • Why use pointers at all? Do **not** use pointers unless you really have to. Pass by value, `std::move` takes care of not creating temp copy. When is the appropriate time to use a pointer? When you are working with a library which you can't alter and which does not have the move constructor and/or assignment operator overloaded, and you want a collection of these objects. And even then, you should use smart pointers instead of raw. – Andy Nov 19 '15 at 21:30
  • If your copy constructor doesn't leak and works correctly, and your destructor doesn't leak and works correctly, an assignment operator need not be coded in the way you have presented it at all. It is much simpler than that. – PaulMcKenzie Nov 19 '15 at 21:31
  • Why is every other post here about implementing linked-list? I don't get it. – LogicStuff Nov 19 '15 at 21:32
  • @LogicStuff School homeworks. – Andy Nov 19 '15 at 21:32

1 Answers1

0

For one thing operator= needs to return T&. You're leaking the memory which aCopy is pointing to. You don't need that at all. You should be doing something like this:

A& A::operator=(const A& a)
{
    this->begin = a.begin;
    //etc.
    return *this;
}
Dan McLeran
  • 39
  • 3