STL Vector issues .

Because you need a destructor, you need a copy constructor for your class and an assignment operator too.

Its the Rule of Three.

This from wiki RuleOfThree:

quote:

---

Any class requiring a destructor, copy constructor, or an assignment operator must have all three implemented

All this gets at is if any of these are written, it can be assumed that there are "extra" resources somewhere in the class that must be dealt with.

---

also STL

Because when you push the object onto the vector you make a copy of it. And as you don''t have a (non-default) copy constructor for your struct, you will have two objects pointing to the same allocated memory, which is freed when the first is destroyed. When the second is destroyed, it tries to free the memory already freed.

You should either make a copy constructor that doesn''t copy the address a, or makes a new allocation. Either will probably require some redesign. You may also want to reconsider the whole thing with this in mind.

thanks , i understood what the problem is . How could i elegantly solve this issue ?

[edited by - Joe Forhens on May 12, 2003 12:00:24 PM]

an example




At (1) the member values of A get assigned to B. You haven''t written your own assignment operator so it is a straight copy. ie the member variable int* a of B gets set to point to what the int* a variables of A is pointing at.

So after line (1) B and A are pointing to the same thing. B has lost track of the 10 integers it was pointing to (memory leak).

At (2) A and B go out of scope and their destructors are called. B''s destructor is called and it deletes the memory it is pointing to. You can see what''s coming. When A''s destructor is called it also tries to delete the same memory. Crash!

Confused about what?

class Ptr
{
public:
    Ptr() : _ptr(new float) {}
    ~Ptr() { if(_ptr) delete ptr; }
private:
    float *_ptr;
};
 
int main(void)
{
    std::vector<Ptr> ptr_list;
    {
        Ptr ptr;
        ptr_list.push_back(ptr);
        ptr_list.pop_back()
    }
    return 0;
}
 

What happens here is that you make a Ptr, that in its ontructor allocates memory (a float) and store the address in itself. When you push it onto the vector, the object (including the pointer to the allocated float) is copied to the vector. When you pop it off the vector, the desctructor is invoked which free the memory (the same memory still referenced by the original ptr). When ptr goes out of scope, its destructor is called, and it tries to free the same memory, which is bad.

[edited by - CWizard on May 12, 2003 12:03:58 PM]

I believe the std::vector class uses the copy constructor and assignment operator (=) of the class you are making a vector of. You have not explicitly defined a copy constructor or an overloaded assignment operator, and so the compiler creates default ones for you. These make shallow copies, where the a member of the object being copied to is assigned to the a member of the object being copied.

This means that when copying objects, you are not actually making a copy of the dynamically allocated memory, but are merely pointing each object''s a member to the same piece of memory. When any of the objects change the memory pointed to by a, it changes the memory for all of the classes, which can easily cause undefined results.

You must define your own copy constructor and assignment operator. These should make deep copies. This means that the function should allocate a separate, yet identical, piece of memory for the a member.

I''ll give an example if you can''t find any information on Google.

[ Google || Start Here || ACCU || MSDN || STL || GameCoding || BarrysWorld || E-Mail Me ]

Yes , thank you - i can clearly see the problem .

Joe are you still wanting an elegant solution or can we move on?

hmmm , the fastest elegant solution i came up with is this :

void asdf::EnablePublicMemoryMode()
{
a = NULL;
}

asdf a;
a.allocate(10);
gg.push_back(a);
a.EnablePublicMemoryMode();

Elentant in terms of easy of modification / small redesigning



[edited by - Joe Forhens on May 12, 2003 12:39:07 PM]

[edited by - Joe Forhens on May 12, 2003 12:39:53 PM]

Joe you just need to write the assignment operator and a copy constructor. Then it will all work elegant.

If you want to share the memory and keep count of how many asdf objects are pointing to it you could do that. You''ll still need to implement the two functions above though.

Yes , you are right - thanks

By the way , i just tested what i said above and it works good . How would you exactly suggest me to do it so that it''s in a more generic form ? Post some code if possible . Also , as long as i don''t want to do direct assignement , should i overload the = operator just for the case of pontential flexibility ?
thanks

class asdf
{
public:
    asdf() : a(0), count(0) {}
    asdf(const asdf &v) : a(0), count(0)
    {
        if(count = v.count) {
            a = new int[count];
            memcpy(a, v.a, sizeof(int) * count);
        }
    }
    ~asdf()
    {
        if(a) { delete a; a = 0; }
    }
    void allocate(int num)
    {
        a = new int[count = num];
    }
private:
    int *a;
    int count;
};

Something to that extent should work. You could use it like this:

std::vector<asdf> asdf_list;
asdf a;
a.allocate(10);
asdf_list.push_back(a);
// or with less waste of memory operations.
std::vector<asdf> asdf_list;
asdf a;
asdf_list.push_back(a);
asdf_list.back().allocate(10); 

You can in this case do without an assignment operator, although you should implement one similar to the copy contructor. However, you example is a bit too general, so I (nor many else) can''t suggest alternate approaches.

I do wish to offer an alternative.
Let''s say you have an object with ''too much internal data'', you don''t want to make a copy constructor for it because it''d be a performance hit. one idea is to disallow the copy of the object.

Here''s a clean way how to do it, according to Scott Meyers:



Continuing on, obviously one couldn''t make a vector with this since the object can''t be copied. Absolutely right, with our goal you should not make a vector of Obj, because recopy constructors would be called too often.

What one would do is use a vector of Obj Pointers.
1-Part of the program somewhere takes care of newing Obj''s and adding the pointers to vector
2-The vector is used normally.
3-Another part of the program makes sure it deletes the pointers in the vector one-at-a-time and NULLs them for safety.

A solution like this is more work, and I advise the use of MMGR to avoid memory leaks, but it enables one to avoid unnecessary constructors. A choice of performance or practicability if you will.

=^.^= Leaders and teachers should remember: It is best to offer others what they Need, not what they Want.

Yes , thank you , i implemented both a copy constructor and a ''='' operator overloading . It works ok - i just ask tehoretically . When''s the copy constructor invoked ? I don''t do it manually ...

quote:

---

Original post by MrNeedHelp
Yes , thank you , i implemented both a copy constructor and a ''='' operator overloading . It works ok - i just ask tehoretically . When''s the copy constructor invoked ? I don''t do it manually ...

---

asdf a;
asdf b(a), c = a; // either

In the first case, the normal constructor, asdf::asdf(), is invoked on a. In the second and third case, asdf::asdf() will never be called on to construct the new objects, but instead asdf::asdf(const asdf &) (aka the copy constructor) should take care of that.