does this vector of pointers using new look ok

[acorn]

What I wanted to do was allocate and store 10 pointers into a vector and then assign some data to it. I wanted to display the data as well as its address and then clean up. does this look ok as far as allocating and cleaning up.

*edit revised it and cleaned it up a bit
*edit changed block from struct to class as i intended to do

#include <iostream>
#include <vector>
#include <limits>

class block
{
public:
	block(int id, int in) : identifer(id), iconindex(in){}

	int  identifer;
	int  iconindex;
};

int main(int argc, char *argv[])
{
	using namespace std;

	vector<block*>library;

	// fill the vector with pointers and data
	for (int i = 0; i < 10; i++)
	{
		int index = i + 1;
		int id = i;

		library.push_back(new block(index,id));
	}

	// print the data
	for (unsigned int i=0; i<library.size(); i++)
	{
		cout << library.at(i)->iconindex << endl;
		cout << library.at(i)->identifer << endl;
		cout << library.at(i)<< endl;
		cout << endl;
	}

	// clean up
	for (unsigned int i=0; i<library.size(); i++)
	{
		delete library.at(i);
		library.at(i) = 0;

		cout << "library at index " << i << " = " << library.at(i) << endl;
	}

	cout << endl;
	library.clear();

	cout << "there is " << library.size() << " elements left." << endl << endl;

	cout << endl;
	cout << "Press ENTER to continue...";
	cin.ignore(numeric_limits<streamsize>::max(), '\n' );
}

[zb0S216C]

A few things:

1) struct members are public by default.

2) When allocating memory, do the allocation operation within a try block. Even if the block is small, the allocation could fail at any time.

Other than these 2 points, I don't see anything immediately wrong.

_Wazzak

[acorn]

Thanks for taking the time to look at it Framework. I didnt notice I left the structure as a structure. I was meaning to change it to a class once i had added the constructor thats why it says public. oops. Im learning more by playing around and trying stuff then just copying whats in a book.

[Athar]

The following line is nonsense: library.at(i) = 0; Remove it.
Aside from that, when you have a container that owns the objects the pointers point to, you should use the ptr_containers (ptr_vector in this case). Boost has an implementation of those.
They automatically delete the objects and due to that, they are exception-safe.

http://www.boost.org/doc/libs/1_46_1/libs/ptr_container/doc/ptr_container.html

[Mathhead200]

Athar wrote:

The following line is nonsense: library.at(i) = 0;

Although it is not needed, it is not "nonsense." It just reinforcing that each pointer will be a NULL pointer after the delete call. However, as far as I know, delete does that already...

[Athar]

delete doesn't do that (what if the argument were an rvalue?).
The assignment is useless because the vector is cleared afterwards.
Setting a pointer to 0 is only necessary when it is still used afterwards and when 0 is a valid value that will be handled correctly.

[acorn]

I set the pointers to 0 after i was through with them because I thought it might be a good programming habit to get in to. not a big deal for me to omit it though.

[Athar]

Well, let's take a step back.

First, dereferencing a null pointer can be expected to always crash the program before anything worse happens.
Dereferencing a pointer that points to an object that has already been deleted is certainly an error, but won't always result in a crash, making it highly dangerous and debugging difficult.

So, if there is a situation that is so complex that even though the pointer should not be dereferenced after deletion, you can't guarantee that it won't happen anyway due to some possible bug in your code, then you should set it to 0 anyway.

Of course, such situations are best avoided in the first place and the use of smart pointers (unique_ptr, shared_ptr) fully takes care of the problem.

In the case at hand, the pointers are destroyed two lines after delete. It's obvious nothing can happen in-between, so setting them to 0 is unnecessary and confusing.

[kempofighter]

Personally, I think that dynamic memory allocation is overkill for this program. I wouldn't bother. I would just copy the objects into the container, and let the container be destroyed at the end of the program. The block class is two integers. I fail to see how dynamically allocating the objects is going to help you.

[acorn]

i did this as an exercise to be sure I understood how certain things are done. I learned a few things so the exercise served its purpose. Why should I make the class bigger then two integers when thats not the point. it was never intended to be the "best" way or anything else. its just an exercise.