Thread: Which of these is a better way to destruct a vector?

Get rid of the char* struct elements and switch to std::string containers. You're making things needlessly complicated.

---

The std::string container requires the <string> header, not the <string.h> header.

---

Do not put using namespace std; declarations in header files. If necessary, explicitly qualify any elements within the header with std:: that need them for the program to compile.

---

There doesn't seem to be a need for all those pointers except perhaps as an experiment. Is this a requirement? You could easily get rid of those pointers and not have to worry about what's in the destructor.

If you really wanted all those pointers, switching the vector elements from raw pointers to some type of smart pointer and implementing a destructor for the Person/FileEntry structs would greatly simplify things. You'd probably only need to call delete on the class data members and be done with it.

Originally Posted by somekid413

doing it this way makes it much easier to really see the effects a particular object has on memory.

In what way? That's interesting, because you should be able to understand everything about memory without pointers.

While it is true that most of the memory your program will use will come from the free store, there is no reason to put STL containers on the free store. They already use it to store whatever they contain. Plus, in C++, memory management isn't supposed to be a big chore.

I'm sorry but you are doing it utterly wrong. It's so blatently wrong it's apalling.

Code:

 *      Author:

Blank huh... Yeah I wouldn't be caught putting my name against that either!

In reality you will never come across a case where it makes sense to have a pointer to a std container as a member of a class. It also does not make sense to store pointers to these very simple objects within the class. It even makes no sense for the members within Person to be pointers, or the ofstream in ClassRoom. You've gone completely mad with pointers.

"doing it this way makes it much easier to make sure everything has a huge negative impact on memory usage and execution speed."
FTFY!

You know how they say "C lets you shoot yourself in the foot and C++ allows you to re-use the bullets"?
Well in your case you're using a rapid-fire chaingun that shoots cannonballs, the trigger is taped down, and your foot is in point blank range.

If the aim here is to do the stupidest thing possible, that will cause the most problems, then you're well on your way to success.

I look forward to reading about this project's failure on theDailyWTF.com in the surely not-too-distant future.

Last but not least, the best way to destruct a vector is:

Code:

// this code block intentionally left blank

Stop me when I'm getting through...

Originally Posted by iMalc

I look forward to reading about this project's failure on theDailyWTF.com in the surely not-too-distant future.

I don't think there's a way to make this funny... :S

yea I recognized that it wasn't a great way to implement the containers.

The fact of the matter is I recently started working for a big company, who's software you probably use, and probably have been using for years. They use software to point out memory leaks in their code, which Identified that a vector pointer to pointers was not getting freed and the memory was being leaked. I'm not saying this to pretend I'm smart. I'm clearly a novice developer, just trying to come up with the best solution to a problem.

I replicated the container in a little bit of demo code and ran it with valgrind to test for mem leaks and In both cases, its really my only option for fixing this as far as I can tell.

If your aim is to write better code, you should do as others in this thread have been suggesting and refactor your example (and the original codebase you're working on) so that it doesn't dynamically allocate pointers to STL containers like vectors. This has a good chance of fixing whatever memory leaks you have. I'm not sure if you know what an std::vector actually contains -- it's basically a size, an allocated size, and a pointer to where the data actually is in memory. So when you dynamically allocate a vector yourself it's like having

Code:

struct Vector {
    int size, allocated_size;
    char *data;
};

Vector *v = new Vector(); // first level of indirection
v->data = new char[1];  // second level of indirection
v->size = v->allocated_size = 1;
v->data[0] = 'x';

You have one level of indirection to the vector, which itself only stores a little more information before invoking yet another level of indirection. This doesn't really make sense. The vector class was designed to be allocated on the stack or to exist as a member variable of your class.

Of course, you may be dealing with a lot of code that uses this unsavoury style and in which case your goal might simply be to fix a memory leak. It shouldn't be necessary to refactor the code just to interpret Valgrind's output. I usually run valgrind with "-v --leak-check=full --show-reachable=yes", I think the "leak-check" option will give fuller stack traces from leaked blocks ("show-reachable" shows leaks stored in static contexts, you may not care about that). Valgrind also has an extremely useful feature where you can record the leaks from one run, then create a suppressions file and re-run with very clean output. Useful if you're experimenting and just want one leak to go away.

Try not to do memory management yourself. You can mess up easily and it's not always simple to hunt leaks down. Instead of using pointers, use the stack or create members inside your class; if you must use pointers, consider std::auto_ptr or the much nicer boost::shared_ptr from Boost, which has also made its way into the new version of C++ (C++11). Might not be useful in this case but something to keep in mind for the future, and for your own projects. And it's not that hard to write yourself.

Code:

template <typename Type>
class AutoDeleter {
private:
    Type *variable;
public:
    AutoDeleter(Type *var) : variable(var) {}
    ~AutoDeleter() { delete variable; }

    const Type *get() const { return variable; }
    Type *get() { return variable; }
};

Then store AutoDeleters instead of pointers, or keep around a vector of AutoDeleters (std::vector<AutoDeleter>) in your class which will automatically delete the class's other variables when it's destructed. That's not really a standard way of doing things, but it's less intrusive. Anyway, I'm sure you can see there are lots of ways to attack the problem of memory-management, and pretty much the worst way is to make all your variables pointers and free them yourself.

Originally Posted by dwks

Try not to do memory management yourself. You can mess up easily and it's not always simple to hunt leaks down. Instead of using pointers, use the stack or create members inside your class; if you must use pointers, consider std::auto_ptr or the much nicer boost::shared_ptr from Boost, which has also made its way into the new version of C++ (C++11). Might not be useful in this case but something to keep in mind for the future, and for your own projects. And it's not that hard to write yourself.

If you have C++0B, it is better to use std::unique_ptr rather than std::auto_ptr.

The class really should disable copy constructor and assignment. If possible, implement move constructor and move assignment.
But then we basically we a unique_ptr.