Dynamically allocated list in C++

Question

I made a cute generic (i.e. template) List class to handle lists in C++. The reason for that is that I found the std::list class terribly ugly for everyday use and since I constantly use lists, I needed a new one. The major improvement is that with my class, I can use [] to get items from it. Also, still to be implemented is an IComparer system to sort things.

I'm using this List class in OBJLoader, my class that loads Wavefront .obj files and converts them to meshes. OBJLoader contains lists of pointers to the following "types": 3D positions, 3D normals, uv texture coordinates, vertices, faces and meshes. The vertices list has objects that must be linked to some objects in all of the 3D positions, 3D normals and uv texture coordinates lists. Faces link to vertices and meshes link to faces. So they are all inter-connected.

For the sake of simplicity, let's consider that, in some context, there are just two lists of pointers: List<Person*> and List<Place*>. Person class contains, among others, the field List<Place*> placesVisited and the Place class contains the field List<Person*> peopleThatVisited. So we have the structure:

class Person
{
    ...
  public:
    Place* placeVisited;
    ...
};

class Place
{
    ...
  public:
    List<People*> peopleThatVisited;
};

Now we have the following code:

Person* psn1 = new Person();
Person* psn2 = new Person();

Place* plc1 = new Place();
Place* plc2 = new Place();
Place* plc2 = new Place();


// make some links between them here:
psn1->placesVisited.Add(plc1, plc2);
psn2->placesVisited.Add(plc2, plc3);

// add the links to the places as well
plc1->peopleThatVisited.Add(psn1);
plc2->peopleThatVisited.Add(psn1, psn2);
plc3->peopleThatVisited.Add(plc3);

// to make things worse:

List<Person*> allThePeopleAvailable;

allThePeopleAvailable.Add(psn1);
allThePeopleAvailable.Add(psn2);

List<Place*> allThePlacesAvailable;

allThePlacesAvailable.Add(plc1);
allThePlacesAvailable.Add(plc2);
allThePlacesAvailable.Add(plc3);

All done. What happens when we reach }? All the dtors are called and the program crashes because it tries to delete things two or more times.

The dtor of my list looks like this:

~List(void)
{
    cursor = begin;
    cursorPos = 0;

    while(cursorPos < capacity - 1)
    {
        cursor = cursor->next;
        cursorPos++;
        delete cursor->prev;
    }

    delete cursor;
}

where Elem is:

struct Elem
{
  public:
    Elem* prev;
    T value;
    Elem* next;
};

and T is the generic List type.

Which brings us back to the question: What ways are there to safely delete my List classes? The elements inside may or may not be pointers and, if they are pointers, I would like to be able, when I delete my List, to specify whether I want to delete the elements inside or just the Elem wrappers around them.

Smart pointers could be an answer, but that would mean that I can't have a List<bubuType*>, but just List<smart_pointer_to_bubuType>. This could be ok, but again: declaring a List<bubuType*> would cause no error or warning and in some cases the smart pointers would cause some problems in the implementation: for example, I might want to declare a List<PSTR> for some WinAPI returns. I think getting those PSTR inside smart pointers would be an ugly job. Thus, the solution I'm looking for I think should be somehow related to the deallocation system of the List template.

Any ideas?

Answer 1

Without even looking at your code, I say: scrap it!

C++ has a list class template that's about as efficient as it gets, well-known to all C++ programmers, and comes bug-free with your compiler.

Learn to use the STL.¹ Coming from other OO languages, the STL might seem strange, but there's an underlying reason for its strangeness, an alien beauty combining abstraction and performance - something considered impossible before Stepanov came and thought up the STL.
Rest assured that you are not the only one struggling with understanding the STL. When it came upon us, we all struggled to grasp its concepts, to learn its particularities, to understand how it ticks. The STL is a strange beast, but then it manages to combine two goals everybody thought could never be combined, so it's allowed to seem unfamiliar at first.

I bet writing your own linked list class used to be the second most popular indoor sport of C++ programmers - right after writing your own string class. Those of us who had been programming C++ 15 years ago nowadays enjoy ripping out those bug-ridden, inefficient, strange, and unknown string, list, and dictionary classes rotting in old code, and replacing it with something that is very efficient, well-known, and bug-free. Starting your own list class (other than for educational purposes) has to be one of the worst heresies.

If you program in C++, get used to one of the mightiest tools in its box as soon as possible.

¹_{Note that the term "STL" names that part of the C++ standard library that stems from Stepanov's library (plus things like std::string which got an STL interface attached as an afterthought), not the whole standard library.}

Answer 2

The best answer is that you must think on the lifetime of each one of the objects, and the responsibility of managing that lifetime.

In particular, in a relation from people and the sites they have visited, most probably neither of them should be naturally made responsible for the lifetime of the others: people can live independently from the sites that they have visited, and places exists regardless of whether they have been visited. This seems to hint that the lifetime of both people and sites is unrelated to the others, and that the pointers held are not related to resource management, but are rather references (not in the C++ sense).

Once you know who is responsible for managing the resource, that should be the code that should delete (or better hold the resource in a container or suitable smart pointer if it needs to be dynamically allocated), and you must ensure that the deletion does not happen before the other objects that refer to the same elements finish with them.

If at the end of the day, in your design ownership is not clear, you can fall back to using shared_ptr (either boost or std) being careful not to create circular dependencies that would produce memory leaks. Again to use the shared_ptrs correctly you have to go back and think, think on the lifetime of the objects...

Answer 3

Always, always use smart pointers if you are responsible for deallocating that memory. Do not ever use raw pointers unless you know that you're not responsible for deleting that memory. For WinAPI returns, wrap them into smart pointers. Of course, a list of raw pointers is not an error, because you may wish to have a list of objects whose memory you do not own. But avoiding smart pointers is most assuredly not a solution to any problem, because they're a completely essential tool.

And just use the Standard list. That's what it's for.

Answer 4

In the lines: // make some links between them here: psn1->placesVisited.Add(plc1, plc2); psn2->placesVisited.Add(plc2, plc3);

// add the links to the places as well plc1->peopleThatVisited.Add(psn1); plc2->peopleThatVisited.Add(psn1, psn2); plc3->peopleThatVisited.Add(plc3);

---

You have instances on the heap that contain pointers to each others. Not only one, but adding the same Place pointer to more than one person will also cause the problem (deleting more than one time the same object in memory).

Telling you to learn STL or to use shared_ptr (Boost) can be a good advice, however, as David Rodríguez said, you need to think of the lifetime of the objects. In other words, you need to redesign this scenario so that the objects do not contain pointers to each other.

Example: Is it really necessary to use pointers? - in this case, and if you require STL lists or vectors, you need to use shared_ptr, but again, if the objects make reference to each other, not even the best shared_ptr implementation will make it.

If this relationship between places and persons is required, design a class or use a container that will carry the references to each other instead of having the persons and places point at each other. Like a many-to-many table in a RDBMS. Then you will have a class/container that will take care of deleting the pointers at the end of the process. This way, no relations between Places and Persons will exist, only in the container.

Regards, J. Rivero

Answer 5

Without looking the exact code of the Add function, and the destructor of your list, it's hard to pin point the problem.

However, as said in the comments, the main problem with this code is that you don't use std::list or std::vector. There are proven efficient implementations, which fit what you need.

Answer 6

First of all, I would certainly use the STL (standard template library), but, I see that you are learning C++, so as an exercise it can be nice to write such a thing as a List template.

First of all, you should never expose data members (e.g. placeVisited and peopleThatVisited). This is a golden rule in object oriented programming. You should use getter and setter methods for that.

Regarding the problem around double deletion: the only solution is having a wrapper class around your pointers, that keeps track of outstanding references. Have a look at the boost::shared_ptr. (Boost is another magnificent well-crafted C++ library).

Answer 7

The program crashes because delete deallocates the memory of the pointer it is given it isn't removing the items from you list. You have the same pointer in at least two lists, so delete is getting called on the same block of memory multiple times this causes the crash.

Answer 8

First, smart pointers are not the answer here. All they will do is guarantee that the objects never get deleted (since a double linked list contains cycles, by definition).

Second, there's no way you can pass an argument to the destructor telling it to delete contained pointers: this would have to be done either via the list's type, one of its template arguments, partial specialization or an argument to the constructor. (The latter would probably require partial specialization as well, to avoid trying to delete a non-pointer.)

Finally, the way to not delete an object twice is to not call delete on it twice. I'm not quite sure what you thing is happening in your destructor, but you never change cursor, so every time through, you're deleting the same two elements. You probably need something more along the lines of:

while ( cursor not at end ) {
    Elem* next = cursor->next;
    delete cursor;
    cursor = next;
}

-- James Kanze

Answer 9

And you could easily implement [] around a normal list:

template <class Type> 
class mystdlist : public std::list<Type> { 
public: 
    Type& operator[](int index) { 
       list<Type>::iterator iter = this.begin(); 

       for ( int i = 0; i < index; i++ ) { 
          iter++; 
       }
       return *iter; 
    }  
};  

Why you would want to do this is strange, IMHO. If you want O(1) access, use a vector.