Link to home
Start Free TrialLog in
Avatar of LearningCpp
LearningCpp

asked on

Memory clean up using pointer to object

I need a basic understanding of how to clean up memory if use pointers and BSTR.

I have the following structure:
struct Value
{
BSTR Key;
BSTR data;
}
struct Element
{
std::vector<Value> val;
std::vector<Element*> childElement;
}

Now in my main program i intialize these sturctues in the following manner:
int main()
{
Element* e = new Element;
Value* v = new Value;
v->Key = SysAllocString(L"Key1");
v->data = SysAllocString(L"Data1");
e->val.push_back(*v);
v = new Value;
v->Key = SysAllocString(L"Key2");
v->data = SysAllocString(L"Data2");
e->val.push_back(*v);

Element* child = new Element;
child->val.push_back(*v);
e->childElement.push_back(child);

child = new Element;
child->val.push_back(*v);
 e->childElement.push_back(child);
 return 0;
}

I hope it does not look complicated.
My query is:
I have created 2 "new" Value but if i try 2 delete statements, the program breaks.
where as i need both the Values till the end, I dont want to create two objects v1 and v2 and then delete, cause it will sacrifice the whole purpose for pointers.

Also I create child = new Element; //twice.
but when i try to delete it twice i cant do it.

Any solution to resolve this, i hope i dont have to change the definations else i have to make a lot of code changes.

Thanks in advance
Avatar of josgood
josgood
Flag of United States of America image

I'm not sure whether I'm answering your question, but here's what I have to say
1)  The code below runs to completion...it doesn't blow up
2)  Memory allocated with SysAllocString is freed with SysFreeString.
3)  Deleting v the first time, along with the SysFreeString releases the memory

int main()
{
   Element* e = new Element;
   Value* v = new Value;
   v->Key = SysAllocString(L"Key1");
   v->data = SysAllocString(L"Data1");
   e->val.push_back(*v);
SysFreeString(v->Key);    //--------------------
SysFreeString(v->data);   //--------------------
delete v;                 //--------------------
   v = new Value;
   v->Key = SysAllocString(L"Key2");
   v->data = SysAllocString(L"Data2");
   e->val.push_back(*v);

   Element* child = new Element;
   child->val.push_back(*v);
   e->childElement.push_back(child);

   child = new Element;
   child->val.push_back(*v);
SysFreeString(v->Key);  //--------------------
SysFreeString(v->data); //--------------------
delete v;               //--------------------
   e->childElement.push_back(child);
delete child;           //--------------------
delete e;               //--------------------
   return 0;
}
Avatar of Infinity08
There is no reason to dynamically allocate the Value's, since you store them in a std::vector.

So, when you do this :

        e->val.push_back(*v);

This means that a copy of the *v object is placed in the vector. The original allocated *v object is still there in memory. And when you subsequently do this :

        v = new Value;

you overwrite the pointer to that old object, creating a memory leak.


The idea would be to create a constructor and destructor for the Value and Element structs, so that cleanup happens automatically, and to not allocate the Value's dynamically, but statically :


struct Value {
  BSTR key;
  BSTR data;
  Value(const OLECHAR *k, const OLECHAR *d) {     // <--- constructor
    key = SysAllocString(k);
    data = SysAllocString(d);
  }
  ~Value() {                                      // <--- corresponding destructor
    SysFreeString(key);
    SysFreeString(data);
  }
}
 
struct Element {
  std::vector<Value> val;
  std::vector<Element*> children;
 
  Element() { }                                   // <--- constructor (nothing to be done)
  ~Element() {                                    // <--- destructor : clean up children
    std::vector<Element*>::iterator it;
    for (it = children.begin(); it != children.end(); ++it) {
      delete *it;
    }
  }
 
  void addValue(const Value &v) { val.push_back(v); }             // <--- some setters
  void addChild(const Element *c) { children.push_back(c); }
}
 
int main() {
  Element* e = new Element;
  e->addValue(Value(L"Key1", L"Data1"));          // <--- add a value using a bit simpler/nicer (?) syntax
  e->addValue(Value(L"Key2", L"Data2"));
 
  Element* child = new Element;
  child->addValue(Value(L"Key2", L"Data2"));
  e->addChild(child);
 
  child = new Element;
  child->addValue(Value(L"Key2", L"Data2"));
  e->addChild(child);
 
  // cleanup of the root Element object needed (children are cleaned by the Element destructor) :
  delete e;
 
  return 0;
}

Open in new window

Avatar of LearningCpp
LearningCpp

ASKER

This look really neat, but when we you say to allocate the Value statically, say if i create 1 element which has 500 child elements and each child element have 10 Values(pairs), wont all the memory be in stack which might cause the program to run slow/stack overflow etc??
however when i have BSTR's in Value, the SysAllocString will allocate memory in heap only rather than stack.
Thanks for the solution, I just need a little clarification so that i can implement the solution.
>> say if i create 1 element which has 500 child elements and each child element have 10 Values(pairs), wont all the memory be in stack which might cause the program to run slow/stack overflow etc??

Remember that when you push_back the Value into the vector, the vector stores a COPY of the Value object. This copy is dynamically allocated. So, there's no problem.
Cool, I have lot of code to update now, I will just finish it by today and if it works, will accept your solution. I am sure it will.

Thanks once again.
Hi,
It does not work, in the sense.
the following code:
>>
1:   e->addValue(Value(L"Key1", L"Data1"));          // <--- add a value using a bit simpler/nicer (?)
2:   e->addValue(Value(L"Key2", L"Data2"));
3:   e->addValue(Value(L"Key3", L"Data3"));
4:   e->addValue(Value(L"Key4", L"Data4"));

After each step 1,2,3,4, when i step through the debugger and see the values of e, i dont find the data bstr's i have been assinging above. because after 1: where i create the Value stuct. as soon as 1: has finished executing, it will call the destructor of Value: ~Value(), which will free the memory which was pointed by e->Value[0]->Key say 18351C, so the next statement will now use this memory, when 2: is executed, the same memory location 18351C is used so e->Value[0]->Key and e->Value[1]->key will have the same memory location, hence my program will fail.
the push_back does not create a copy of the string, maybe because its a BSTR, hence it stores only the address

Infinity08, can you provide a solution for this??


ASKER CERTIFIED SOLUTION
Avatar of Infinity08
Infinity08
Flag of Belgium image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
This is getting interesting: {weird} increasing the points after getting solution as i need more info.
I have the complete listing of the structure below.
I had to add a default constructor Value() {} because at lot of places I have defined Value as, Value v;
so it will need a default constructor.
now when the function gets over where i defined this Value, it will call the ~destructor for Value()
which will try to SysFreeString(Key). and throws an error.
How do i check if the BSTR is already freed (Infinity08, if you feel that this question is out of the scope of the original question, then i can accept this solution and post a new query)

Thanks for the clarification
struct Value {
  BSTR key;
  BSTR data;
  Value() {} //Added this default constructor cause i have Value v; at
             // many places
  Value(const OLECHAR *k, const OLECHAR *d) {     // <--- constructor
    key = SysAllocString(k);
    data = SysAllocString(d);
  }
  ~Value() {                                      // <--- corresponding destructor
    SysFreeString(key);
    SysFreeString(data);
  }
 
 
Value(const Value &v) {     // <--- copy constructor
    key = SysAllocString(v.k);
    data = SysAllocString(v.d);
  }
 
  Value &operator=(const Value &v) {      // <--- assignment operator
    if (this != &v) {
      SysFreeString(key);
      SysFreeString(data);
      key = SysAllocString(v.key);
      data = SysAllocString(v.data);
    }
    return *this;
  }

Open in new window

>> (Infinity08, if you feel that this question is out of the scope of the original question, then i can accept this solution and post a new query)

No problem ;) Still on topic ... and happy to help :)


>> which will try to SysFreeString(Key). and throws an error.

Yes, if you add a default constructor, you will have to account for the fact that the BSTR's might not have been allocated.
The easiest way would be to either initialize the BSTR's to NULL in the default constructor, or to initialize them to an empty string, so either of these depending on what you prefer :

        key = NULL;
        key = SysAllocString(L"");
Agreed, would go with the NULL method.
Thanks for the solution