1

I am implementing a dual-key (int and string) lookup table in C++ and would like to know how I should write a destructor for it.

The lookup table API allows users to insert and lookup a named object into the table. Under-the-covers, a unique int key is automatically assigned to the string which holds the name of the object. After insertion the inserted object can be looked up by its string name or int value.

Here is the gist of the implementation.

template <class VALUE>
class COLLECTION
{
    public;
    typedef int IDX;
    typedef std::map< std::string , IDX >   NAME_TO_IDX;
    typedef std::map< IDX, std::string >   IDX_TO_NAME;
    typedef std::map<IDX, VALUE*>          IDX_TO_VAL;
...
    void insert( const std::string name, VALUE* v )
    {   
        m_name_to_idx_map[name]       = m_next_idx;
        m_idx_to_name_map[m_next_idx] = name;
        m_idx_to_val_map[m_next_idx] = v;
        ++m_next_idx;
    }
    VALUE* get_value( const IDX idx ) { return m_idx_to_val_map[idx]; }
    VALUE* get_value( const std::string name )
    ....
    private:
    int           m_next_idx;
    NAME_TO_IDX   m_name_to_idx_map;
    IDX_TO_NAME   m_idx_to_name_map;
    IDX_TO_VAL    m_idx_to_val_map;

How would I write a destructor for this class? Thanks

15
  • 1
    Absolutely: void insert(const std::string& key, std::unique_ptr<VALUE> value), bingo. If you hate expressing requirements in code, you could also pass a raw pointer and add a comment above the function declaration that says // Takes owernship of *value and calls "delete" on the pointer.. (It's important to be clear that users must not, say, pass in pointers obtained from malloc or from array-new, so just "takes ownership" isn't precise enough. But the unique pointer leaves no ambiguities.)
    – Kerrek SB
    Dec 29, 2015 at 2:13
  • 1
    Better (on average) to use a vectors instead of maps for m_idx_to_name_map and m_idx_to_val_map, as the indices are contiguous (assuming you don't support deletions). Dec 29, 2015 at 2:13
  • 1
    It's also questionable why you would have this seemingly pointless indirection and the conflated responsibilities of your class. Why not just have a map<int, VALUE>? If the user cares for indirection, she can use VALUE = std::unqiue_ptr<X>.
    – Kerrek SB
    Dec 29, 2015 at 2:16
  • 1
    You might also consider boost::bimap for such bi-directional mappings. Dec 29, 2015 at 2:19
  • 1
    Kerrek's suggestion to separate responsibilities is by far the best option here, and unique_ptr a sound second, but FWIW another is to have a template policy parameter or constructor argument specify a functor that can be invoked to deallocate (if necessary) the VALUE*. Separately, it's best practice to reserve all-uppercase identifiers for preprocessor macros, excepting single-letter identifiers which can be used for template parameters. Dec 29, 2015 at 2:37

2 Answers 2

0

To avoid writing manual deallocating memory owned by all VALUE* in destructor - probably you should use smart pointers.

C++11 provides std::shared_ptr which you can use for storing VALUE*,

typedef std::map<IDX, std::shared_ptr<VALUE> > IDX_TO_VAL;

If you can use boost library then you can use boost::shared_ptr.

0

The only heap allocated portion is possibly the VALUE* (Not a huge fan of the uppercases). For your destructor, you have have to loop through the map and delete all of the relevant pointers as follows

for(auto iter = IDX_TO_VAL.begin(); iter != IDX_TO_VAL.end(); ++iter)
{
    delete (*iter).second;
}
1
  • @unshul What if VALUE points to something allocated on the heap? -- And what if it doesn't? You have undefined behavior if you attempt to call delete on that value. That's the flaw in your design -- you can't assume where that address comes from that you're populating your map with. Dec 29, 2015 at 2:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.