# Creating objects and putting them into std::list

I am writing small roguelike game where I spawn some items and put them into the std::list. Is this the right way to do it?

The Item class is as followed (I know it has public id property):

class Item {

public:
Item();
~Item();

unsigned int id;
};


Somewhere in the code I also defined:

list<Item*> listOfItems;
list<Item*>::iterator it;


Now I am using this code to create an item:

Item *p = new Item();
p->id = itemCrowbar;
listOfItems.push_back(p);


At the end of the program I use small loop to delete all items:

for (it = listOfItems.begin(); it!=listOfItems.end(); it++) {
delete *it;
}


Is this approach for creating objects, putting them in std::list and at the end deleting them correct, or am I missing something important here?

-

If Item has subclasses, then make the ~Item() destructor virtual.

If Item has a copy constructor, and is not a superclass of a subclass, then you can store a copy of the item (not pointer to the item) in the list:

list<Item> listOfItems;

Item item;
item.id = itemCrowbar;
listOfItems.push_back(item);


In that case, you won't need to delete Item pointers before the list is destroyed.

However, if Item has subclasses, then this might be legal but a bug (called 'object slicing'):

list<Item> listOfItems;

SubclassOfItem item;
item.id = itemCrowbar;
item.extra = "hello";
listOfItems.push_back(item);


Or you can create a list of 'smart pointers' (std::unique_ptr) to items: which behave like pointers to Items except that you don't need to delete the Item (the Item is deleted when the smart pointer which contains it is destroyed).

-
Never, ever store std::auto_ptr in a container. Further, auto_ptr is deprecated, so it shouldn't be used anyway. – Yuushi Feb 13 '14 at 1:49
@Yuushi Not even in a std::list? I imagined that list nodes would be immobile. – ChrisW Feb 13 '14 at 1:52
"Copying an auto_ptr copies the pointer and transfers ownership to the destination: both copy construction and copy assignment of auto_ptr modify their right hand arguments, and the "copy" is not equal to the original. Because of these unusual copy semantics, auto_ptr may not be placed in standard containers." See here. – Yuushi Feb 13 '14 at 1:53
Item will have subclasses and won't have any copy constructor. – Michal Przybylowicz Feb 13 '14 at 2:22
@MichalPrzybylowicz Even without a copy-constructor there might (in modern C++) be a move constructor; but if there are several subclasses then you're right that you need to store by pointer, not by value. Don't forget to make your destructor virtual. And consider using a mart pointer to avoid the need to do explicit destruction. – ChrisW Feb 13 '14 at 12:04

No, it is not correct.

For such classes, you should do not need to create objects on the heap. This should do :

Item p;
p.id = itemCrowbar;
listOfItems.push_back(p);


If you add a constructor with a parameter like this ...

class Item {

public:
Item(unsigned int id_):id(id_){}

unsigned int id;
};


... then you could even do this :

int main()
{
std::list<Item> items;

unsigned int someId = 5;

items.push_back( {someId} );
}


This way you do not have to wory about memory leaks, and you can remove the loop releasing created objects.

-