std::map, references, pointers and memory allocation

I am having a lil hard time with map and the valuetype allocation.

consider this simple class:

class Column {
private:
    char *m_Name;
public:
    // Overrides
    const char *Name(){
        return this->m_Name;
    }

    // Ctors
    Column(const char *NewName){
        this->m_Name = new char[strlen(NewName) + 1];
        strcpy(this->m_Name, NewName);
    }

    // Dtors
    ~Column(){
        cout << "wtf?\n";
        delete this->m_Name;
    }
};

now I have this map:

// Typedefs
typedef std::map<int, Column> ColumnContainer;
ColumnContainer *m_Container;

When i call this:

Column *c = new Column("Test");
cout << "CREATED: " << c->Name() << "\n";
it = this->m_Container->insert(st开发者_Go百科d::make_pair(0, *c)).first;
cout << "AGAIN: " << c->Name() << "\n";

the console is printing the "wtf?" after the insert in the map.

it seems to be destroying the column. Is this right?

or am I doing something wrong?

I was wondering if the value_type of the std::map has to a struct type with defined memory size, like with POD or array of POD?

the cout << AGAIN doesn't print the "Test"

what I need is a map to a columns based on int key

make_pair(0, *c) creates a (temporary, unnamed) pair<int, Column>. This pair is passed to map::insert by reference, and, since std::map owns its elements, it makes a copy of the pair. Then, the temporary pair is destroyed, destroying the Column object it contains.

This is why it is necessary to properly define copy-construction for types that you want to use as standard container elements.

What is happining in your code:

Here you are dynamically creating an object. (this is not freed in your code).
Why are you using a pointer? (Smart pointer are your friend.)
But a normal object would be preferable.

Column *c = new Column("Test");

Now you make a copy of the object as it is copied into the temporary object of type std::pair<int,Column> (This uses Column's copy constructor which makes a copy of the M_name member (you now have two pointers to the same memory location)).

std::make_pair(0, *c)

Now you insert the pair<int,Column> into the map. This is done by again using the Column copy constructor (it uses the make_pair copy constructor which calls the Column copy constructor). Again the M_name pointer is copied into this object. So now you have three objects poining at the dynamically allocated string.

m_Container->insert( pairObject )

At this point the temporary object you create with the call to std::make_pair() is no longer needed so it is destroyed. This is where you destructor is getting called. Of course this leaves you with two objects with pointer at memory that has now been released.

You have a big problem.

You either need to use std::string (preferred solution)
Or you need to correctly handle the RAW owned pointer in your class. This means you need to implement all four of the defaultly generated methods:

• constructor
• destructror
• copy constructor
• assignment operator

Small problem:

You need to stop coding like a java programmer.

Column *c = new Column("Test");
it = this->m_Container->insert(std::make_pair(0, *c)).first;

Should look like this:

m_Container[0] = Column("Test");

There is no need to dynamically allocate everything.
Infact that is very dangerious.

Explanation for why having a RAW owned pointer is a bad idea.

class X
{
    char*   m_name;
  public:
    X(char const* name)  {m_name new char[strlen(m_name) +1];strcpy(m_name,name);}
    ~X()                 {delete [] m_name;}
};

This looks fine. But the compiler is also generating two methods for you. In mnost situations this is fine but not when you have a RAW owned pointer.

X::X(X const& copy)
    :m_name(copy.m_name)
{}

X& X::operator=(X const& copy)
{
    m_name = copy.m_name;
}

Now image the code:

X    x("Martin");
X    y(x);

Both 'x' and 'y' now contan pointers (m_name) pointing at the same piece of memory. When 'y' goes out of scope it calls its derstructor which calls the delete [] on the memory. Now 'x' goes out of scope and calls delete on the same piece of memory.

Z    z("Bob");
z = x;

Same problem as above jsut using a different operator.

How does this apply to you?
You are using a map of pointer to Column. The map actually stores a Coloumn object. So it is using the Copy constructor above to make a copy of your object. So there is a probelm. But also in code there is a lot of times when temporary objects are created and passed around.

doWork(Column const& x) { /* Do somthing intersting */

doWork(Column("Hi There"));

Here we create a temporary Column object that is passed to the doWork(). When doWork() is complete the temporay goes out of scope and is deleted. But what happens if doWork() makes a copy of the object using either the copy costructor or assignment operator? This is what is happening when you insert the object into the map. You are creating a temporary pair then copying this value into the map. This temporarty pair is then being destroyed.

The underlying reason why your string m_Name doesn't print the second time is because of the way the STL builds a map. It makes various copies of the value during its insertion. Because of this, m_Name gets destroyed in one of the copies of the original column.

Another piece of advice is to use pointers to objects when the object is a value in the map. Otherwise you could be taking a major performance hit of the object is large enough.

The m_Name in your container should be a string, so it can be copy-constructed into a map.

Right now you are not defining a proper copy constructor, so it's only copying m_Name as a pointer, which is invalid.

try simplifying by doing

class Column {
private:
    std::string m_Name;
public:
    // Overrides
    const char *Name(){
        return m_Name.c_str();
    }
};

You get the copy constructor for free thanks to C++ and all your members being copy constructible.

What you're seeing is that a temporary copy of Column is being destroyed. If you instrument the constructor you should see the copy being created.

Why not using a std::string for the Column name ? Really ? And then everything's alright.

Because here you've got a number of problems... beginning with your destructor (use delete[] when allocation is done by new[])

Also, do you really need to create your column with new ?

Let's rewrite that:

class Column
{
public:
  Column() : m_name() {}
  Column(const std::string& name) : m_name(name) {}

  const std::string& getName() const { return m_name; }

private:
  std::string m_name;
};

And now your insertion code:

std::map<int,Column> m_container;

Column myColumn = Column("Test");
std:cout << "CREATED: " << myColumn.getName() << std::endl;
m_container[0] = myColumn; // COPY the column
std::cout << "AGAIN: " << myColumn.getName() << std::endl;

And here everything is fine. And even slickier syntax

m_container[0] = Column("Test");

C++ already requires a fair amount of code bloat, let's use the shortcuts it offers whenever possible.