thought I understood scope... guess not

I create items in a while loop:
MyStruct *temp = new MyStruct;
I add some data and then I put the items in a vector with much higher scope:
MyVector.push_back(*temp);

For a single varaible this works find but when I add the second variable the first varaible's destructor is called. I thought that if a variable is created as a pointer and new is used to allocate it, its destructor isn't call until I explicitly call delete or MyVector.clear().
What am I missing?

Quote:

---

Originally posted by pim42
I create items in a while loop:
MyStruct *temp = new MyStruct;
I add some data and then I put the items in a vector with much higher scope:
MyVector.push_back(*temp);

For a single varaible this works find but when I add the second variable the first varaible's destructor is called. I thought that if a variable is created as a pointer and new is used to allocate it, its destructor isn't call until I explicitly call delete or MyVector.clear().
What am I missing?

---

Please post all the code that shows your problem. I don't see a while() loop, and I don't see a call to vector.clear(). Also, vector.clear() does not and cannot call delete for your pointer. How does the vector know that the pointer you placed in it was dynamically allocated? You could have done this:

Code:

---

MyStruct t;
MyVector.push_back(&t);

---

Regards,

Paul McKenzie

Well I was trying to ask a generalized question(s).

a). If I create a variable like above:
MyStruct *temp = new MyStruct;
in a function, the pointer "temp" will be freeded when the function goes out of scope but the MyStruct it points to will not be freeded. So the destructor of MyStruct should not be called. Is this correct?

b).For some reason when I call:
MyVector.push_back(*temp);
for the second time, the destructor of the first element is called. I do not understand this. With the debugger I have traced the error to line 162 of VECTOR (VC++ 6) which is:
_Destroy(_First, _Last);
I guess this is really vague but I really don't know what else to say.

This really is all the code that has to do with the problem other than the classes themselves. All my classes are are POD with BYTE * and a destructor for the dynamic BYTE arrays formed from the BYTE *s. My classes do not have copy constructors, I think that might be the problem.

Also I did not say that vector.clear() would call delete but rather destructors. I was under the impression that anytime a classes was deleted (via delete or scope or in this case vector.clear()) that's class destructor is called.

Quote:

---

Originally posted by pim42
Well I was trying to ask a generalized question(s).

a). If I create a variable like above:
MyStruct *temp = new MyStruct;
in a function, the pointer "temp" will be freeded when the function goes out of scope but the MyStruct it points to will not be freeded. So the destructor of MyStruct should not be called. Is this correct?

---

The pointer "temp" is no longer available when you leave the scope, but the memory that you allocated has not been disposed of. Since you did not call "delete" for your "new", the destructor is not called.

Quote:

---

b).For some reason when I call:
MyVector.push_back(*temp);
for the second time, the destructor of the first element is called.

---

This is why you should post a relevant example. What do you mean by "second time"? A small example showing this behavior is all you need to post. For example, this works correctly:

Code:

---

#include <vector>

int main()
{
    std::vector<int*> pIntV;
    int *p = new int;
    pIntV.push_back( p );
    pIntV.push_back( p );  // second time
    delete p;
}

---

There is no problem with this code, and this is my interpretation of a "second time". The reason why you should post an example showing the behavior is two fold.

1) So that we don't guess or try to interpret what you are doing (for example, the code above I posted).
2) In the process of duplicating the problem with a small example, you may discover your own bug.

Quote:

---

I was under the impression that anytime a classes was deleted (via delete or scope or in this case vector.clear()) that's class destructor is called.

---

Yes, the destructor of your class is called. It would also be relevant if you posted MyStruct. Does it contain members that are pointers to dynamically allocated memory? If so, does MyStruct contain proper copy, assignment, and a destructor to handle these members?

Regards,

Paul McKenzie

Your classes do have copy ctors - it's just that you haven't written them, the compiler has. And it sounds like the compiler's version is wrong.

You seem to have a classic case of the "rule of three" here: if you need one of: dtor, copy ctor or copy assign, then you probably need all three. Since your dtor frees dynamic memory, then you definitely want to either create a proper copy ctor and copy assign or disable those two.

Code:

---

class Dynamic
{
    BYTE* something;
public:
    ~Dynamic(); // Frees memory
    Dynamic(const Dynamic&); // Allocates new memory for the copy.
    Dynamic& operator=(const Dynamic&); // Ditto
};

class DynamicNoCopy
{
    BYTE* something;
public:
    ~DynamicNoCopy(); // Frees memory

private:
    DynamicNoCopy(const DynamicNoCopy&); // No implementation
    void operator=(const DynamicNoCopy&); // No implementation
};

---

Note that std::vector requires your class to be copyable. What you are seeing is the destruction of intermediate copies - copies that haven't been made properly. They all point to the same dynamic memory - the first one to be destroyed will delete that memory - the next one will try to delete it again. I'll leave you to ponder on the meaning of "undefined behaviour".

Quote:

---

Originally posted by Graham
Note that std::vector requires your class to be copyable.

---

There is my problem.

Quote:

---

Originally posted by pim42


There is my problem.

---

Which is why it was important to post the MyStruct class. If it contained pointers to dynamic memory, copying, asssingment, and destruction must be coded correctly. Basically, if your class can't pass this test, it is not suitable to be placed in a vector, or even suitable to be used in a non-trivial program without usage of vector:

Code:

---

int main()
{
  MyStruct a;

  // Assume a has some data in it
  MyStruct b = a;  // copy constructor
  MyStruct c;       
  c = a;  // assignment operator
}  // destruction

---

With this small snippet of code, you would more than likely have duplicated your problem, without usage of vector.

I also didn't see that you were placing objects in the vector, so excuse my mistake there.

Regards,

Paul McKenzie

ok more more quick question (I'm using my text book, C++ How to Program by Deitel, as a reference and it only has 2 lines on assignment operators!). Since the assignment operator is almost the same thing as the copy ctor can I call the copy ctor from the operator=? if so how?

Code:

---

class MyClass
{
public:
    MyClass(){} //after creating a copy ctor compiler complained about no ctor so I created a blank one
  ~MyClass()
    {
        free(MyString);
    }
  MyClass(const MyClass& p)
  {
        other = p.other;
        byteLen = p.byteLen;
        MyByteStringCpy(MyString,p.MyString,byteLen);
  }
  MyClass& operator=(const MyClass& p)
  {
        if(this != &p)
        {
            this->~MyClass();
            this->MyClass(p);  //this is wrong but I'm wondering if I can do something like this
        }
        return *this;
  }

    int other;
    BYTE byteLen;
    BYTE* MyString;
};

---

A lot of people call operator= from their copy constructor:

Code:

---

class A
{
public:
  A() {}
  A(const A& rhs)
  {
      *this = rhs;
  }

  const A& operator=(const A& rhs)
  {
      if (this != &rhs)
      {
        // assign members
      }
  }
};

---

That makes sense but creates another problem for me! It just never ends.

PaulWendt you left out the part that is giving me problems now... in the operator= you have to delete (in my case free) any already allocated memory.

Now again, I want to see if I understand correctly. My constructor (which I have added initialize of ptrs to NULL) should be called twice before a copy ctor is ever called. Is this correct? That does not seem to be the case though and that confuses me.

So I'm having the problem of trying to free a pointer to 0xcdcdcdcd when it should be NULL and skip the delete.

Personally, I call the copy ctor from the copy assign:

Code:

---

class foo
{
public:
    foo(const foo&);
    foo& operator=(const foo& rhs)
    {
        foo tmp(rhs);
        swap(tmp);
    }
private:
    void swap(foo&); // non-throwing function to swap
                      // member data of *this with argument.
};

---

This has the advantage of being exception-safe, and not requiring a check for self-assignment. But you do need a non-throwing swap function (which is not too difficult achieve, actually).

Quote:

---

Originally posted by pim42
That makes sense but creates another problem for me! It just never ends.

PaulWendt you left out the part that is giving me problems now... in the operator= you have to delete (in my case free) any already allocated memory.

Now again, I want to see if I understand correctly. My constructor (which I have added initialize of ptrs to NULL) should be called twice before a copy ctor is ever called. Is this correct? That does not seem to be the case though and that confuses me.

So I'm having the problem of trying to free a pointer to 0xcdcdcdcd when it should be NULL and skip the delete.

---

Well, you're initializing in your constructor. A copy constructor is
also initializing so you'd ideally be initializing these pointers to 0
there. The assignment operator takes an EXISTING object and
severs its ties to its old life, giving it a whole new value. The
copy constructor actually constructs the object itself. So, you
would do something like this with my method:

Code:

---

class A
{
public:
  A() : m_p(0) {}
  A(const A& rhs) : m_p(0)
  {
      *this = rhs;
  }

  const A& operator=(const A& rhs)
  {
      if (this != &rhs)
      {
        // assign members
      }
  }

private:
  B* m_p;
};

---

Alternatively, you could do something like this:

Code:

---

class A
{
public:
  A()
  {
      initialize(); 
  }

  A(const A& rhs) : m_p(0)
  {
      initialize();
      *this = rhs;
  }

  const A& operator=(const A& rhs)
  {
      if (this != &rhs)
      {
        // assign members
      }
  }

private:
  void initialize()
  {
      m_p = 0;
  }

  B* m_p;
};

---

Personally, I like the first approach better.

You could also use Graham's swap() method; I typically don't
employ that method unless there's a definite speed advantage
[like std::vector], but it's definitely something a ton of people like
so I might be the "odd man out" so to speak.

--Paul