0

I'm building a hashmap class that can have string keys and ints, bools, strings or pointers of different types as its values, and I want it to work. For the program I'm using it for I create the pointer and pass it into the hashmap. The problem comes when I need to destruct the map. If the type for the hashmap is a pointer I need to delete it(the value) before I delete it's container.

so the code I have right now goes something like this: I have a hashNode** drawers, which I use as a two dimensional array to hold pointer to hashNodes in the map. Those same pointers are also held in another hashNode** array, which stores them as they are added to map (for ease/speed of growing and copying the hashmap).

template <typename V>
class str_map {
public:
    // ...
    virtual ~str_map() {
        str_map<V>::~str_map();
    }
    // ....
};

and then later I have a bunch of methods like these: one for regular values:

template <>
str_map<int>::~str_map() {
    for(int i=0; i < count && array[i] != NULL; i++){
        delete array[i];
    }
    delete array;
    delete drawers;
}

and one for pointers:

template <>
str_map<str_map<int>*>::~str_map() {
    for(int i=0; i < count && array[i]->val() != NULL; i++)
        delete array[i]->val();
    for(int i=0; i < count && array[i] != NULL; i++){
        delete array[i];
    }
    delete array;
    delete drawers;
}

Is there another better way to deconstruct an instance of str_map class correctly so that all the memory is handled correctly? Or at least a way to make this work?

Trevor
  • 955
  • 9
  • 16
  • 3
    This is probably the most frightening code I've seen all month: `virtual ~str_map() { str_map::~str_map(); }`. – GManNickG Nov 10 '10 at 00:40

1 Answers1

1

Your container should handle values. That's it, no more, no less. If someone wants to stick pointers in, let them. Don't take ownership of whatever they may or may not be pointing at.

It's up to the users of your hash map to know how to manage the lifetime of their memory. They should be using smart pointers, so your class just copies them around and the smart pointer manages the memory.

The guideline is manage one resource, or none at all. If you are managing more than one resource, you've set yourself up for failure.

I suspect delete array should be delete [] array;. What this means is you really should be using std::vector. Again, either manage one resource or none at all. std::vector manages one resource, so you don't have to. And so on.

GManNickG
  • 494,350
  • 52
  • 494
  • 543
  • then the only way for the person using the class to manage memory properly passing in a pointer is to pass in a smart pointer? – Trevor Nov 10 '10 at 00:47
  • @Trevor: This is outside the situation. In modern C++ code, you never have a raw `new` lying around; it's just bad practice. Someone *could* add an `int*` initialized with `new int()` to your class, or they could add an `int*` initialized with `int j = 5; i= &j;`; it's impossible for your container to tell, and you shouldn't try to manage anything outside of what it takes to manage the values you're storing. The former code (`int *i = new int()`) is terrible, though. It would look something more like `std::shared_ptr i(new int());`; this never leaks. – GManNickG Nov 10 '10 at 00:55
  • Ok. Thanks. I've known about shared pointers but haven't started using them yet. Now's as good a time as any. I guess I should have mentioned that I'm new to C++, although it probably goes without saying ... – Trevor Nov 10 '10 at 00:59