Solved

memory allocation/deallocation for a vector of pointers

Posted on 2004-09-02
16
955 Views
Last Modified: 2013-12-14
I have code like the following:

vector <Widget *> GetWidgets(){
   vector <Widgets *> widgets;
   for(i=0;i<n;i++){
     Widget * widget = new Widget();
     widgets.push_back(widgets);
}

void DeleteWidget(Widget*& widget_p){
    if(widget_p){
       delete widget_p;
       widget_p = 0;
  }
}

main(){
  for(some loop){
    vector <Widget *> widgets = GetWidgets();
      ... do stuff with widgets...
 
     for_each(widgets.begin(), widgets.end(), DeleteWidget);
  }
}


------
Before I added the last line, I understandably had a memory leak.
Now, the code runs and the memory leak seems to be gone,  but I get the
following message during run time:

*** malloc[24383]: Deallocation of a pointer not malloced: 0x2809420; This could be a double free(), or free() called with the middle of an allocated block; Try setting environment variable MallocHelp to see tools to help debug
staring loop contents

It seems that I am only getting this message for one widget each run of the loop (the rest delete without the error)
What's going on here, how do I fix it, or is there a much better way altogether to delete my vector of
widget pointers?

Thanks!
0
Comment
Question by:axmeno
  • 5
  • 3
  • 2
  • +3
16 Comments
 
LVL 11

Accepted Solution

by:
avizit earned 100 total points
ID: 11968490
can you try this i

vector <Widget *>::iterator vec_iterator;

vec_iterator = my_vector.begin();

while (vec_iterator != my_vector.end()){
    DeleteWidget( *vec_iterator);  
    ++vec_iterator;
}


instead of

 for_each(widgets.begin(), widgets.end(), DeleteWidget);
0
 

Author Comment

by:axmeno
ID: 11968562
No dice.  This seems to do the same thing -- same memory usage and same error message.
0
 

Author Comment

by:axmeno
ID: 11968680
Left out a piece of information that may be relevant.

one peice  of the "do stuff to widgets" was

for_each(widgets.begin(), widgets.end(), DeleteBadWidget);
widgets.erase(remove(widgets.begin(),widgets.end(),static_cast<Widget*>(0)), widgets.end());

void DeleteBadWidget(Widget*& widget_p){
    if(some condition){
       delete widget_p;
       widget_p = 0;
  }
}

Just one bad widget is getting deleted here.  This is probably the one that's giving the error message -- am I deleting
it a second time?  How do I get it out of the widgets vector the first time, so I don't do that?

If I take out the widgets.erase line, or replace these two lines with the code suggested above as well, I get a bus error.
I'm not completely clear on what this line is doing anyways...can someone explain?

Thanks!


0
 
LVL 30

Assisted Solution

by:Axter
Axter earned 100 total points
ID: 11968835
Try copying and pasting your actual code here, so we can see exactly what your using.

The code you posted in your question has many compile errors, so I'm sure it's not a copy and paste of your original code.
0
 
LVL 11

Expert Comment

by:avizit
ID: 11968839
but in your

void DeleteWidget(Widget*& widget_p){
    if(widget_p){
       delete widget_p;
       widget_p = 0;
  }


you are checking if widget_p is 0 . so if it was deleted earlier the code wont do a  delete widget_p; on this..

anything I think you can put a cout<< on both the function

DeleteWidget() and DeleteBadWidget()
 
to see your program abended in which function .





0
 

Author Comment

by:axmeno
ID: 11968846
The last comment on the intermediate removal was a red herring, I've removed it and still have the error message.  I'll try to directly extract the actually relevant code and paste it.
0
Highfive Gives IT Their Time Back

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 
LVL 30

Expert Comment

by:Axter
ID: 11968847
Modified version of your code, which does work to create and delete all instances, and therefore has no memory leaks.

int QtyExistingInstance = 0;

class Widget
{
public:
      Widget()
      {
            ++QtyExistingInstance;
      }
      ~Widget()
      {
            --QtyExistingInstance;
      }
};

vector <Widget *> GetWidgets(int n){
  vector <Widget *> widgets;
  for(int i=0;i<n;i++){
    Widget * widget = new Widget();
    widgets.push_back(widget);
  }
  return widgets;
}

void DeleteWidget(Widget*& widget_p){
   if(widget_p){
      delete widget_p;
      widget_p = 0;
 }
}

int main(){
 for(int i = 0;i < 3;++i){
   vector <Widget *> widgets = GetWidgets(11);
     for_each(widgets.begin(), widgets.end(), DeleteWidget);
 }
 cout << "Value of QtyExistingInstance = " << QtyExistingInstance << endl;
 return 0;
}
0
 
LVL 30

Expert Comment

by:Axter
ID: 11968859
If you have a memory leak, it's more then likely caused by code you still have not shown us yet.

Please copy and paste the code in a comment, and post it so we can see **exactly** what you're compiling.
0
 

Author Comment

by:axmeno
ID: 11969152
Okay, here' s the essense of the code.  This compiles by itself and generates the error  (though, it runs just fine, outputting this error, as does
my more complex code).  Suggestions on how to do this in a more sensible way are welcome.


#include <vector>
#include <algorithm>

using namespace std;

class Galaxy{
 public:
  Galaxy(){};
};

void Delete(Galaxy*& pgalaxy){
  if(pgalaxy){
    delete pgalaxy;
    pgalaxy =0;
  }
}

vector <Galaxy *> GetGalaxies(){
  vector <Galaxy *> galaxies;
  Galaxy * g = 0;
  int ngals = 100;
  for(int i=0;i<ngals;i++){
    g = new Galaxy();
    galaxies.push_back(g);
  }
  delete g;
  return galaxies;  
}

int main(){
  vector <Galaxy*> galaxies = GetGalaxies();
  for_each(galaxies.begin(),galaxies.end(),Delete);
  galaxies.erase(remove(galaxies.begin(),galaxies.end(),static_cast<Galaxy*>(0)), galaxies.end());
}

0
 

Author Comment

by:axmeno
ID: 11969172
Aha!  For some reason I had an extraneous delete g in there.  All is well now.
Anyways, if someone has something worthwhile to say about the basic way I'm
doing things above, I'm still happy to award some points.

Cheers...
0
 
LVL 3

Assisted Solution

by:Indrawati
Indrawati earned 100 total points
ID: 11969252
void Delete(Galaxy*& pgalaxy){
  if(pgalaxy){
    delete pgalaxy;
    pgalaxy =0;
  }
}

C++ delete already checked if the pointer is null before deleting it, so there's no need for you to check it again. So maybe you can change it to:
void Delete(Galaxy*& pgalaxy){
    delete pgalaxy;
    pgalaxy =0;
}


vector <Galaxy *> GetGalaxies()
instead of returning by value, maybe you can return by const reference instead to prevent unnecessary copying? Something like: const vector <Galaxy *>& GetGalaxies()

galaxies.erase(remove(galaxies.begin(),galaxies.end(),static_cast<Galaxy*>(0)), galaxies.end());
I don't understand this... You have already performed for_each and delete on the line before it, so all the elements in galazies should be zero now. If you want to erase all the elements in the vector, you can use galazies.clear();
0
 
LVL 3

Assisted Solution

by:teratoma
teratoma earned 100 total points
ID: 11978076
If you used boost::shared_pointer (www.boost.org) you wouldn't have to delete anything.
0
 

Expert Comment

by:s_senthil_kumar
ID: 12029659
How about a typedef for vector<Widget *>

typedef vector<Widget *> WidgetList;

1. That helps to improve code readability, you can write
WidgetList GetGalaxies() {...}

2. It allows you to change it from a vector to something else, say a list, whithout modifying code at a hundred places.

And Indrawati, the author can't return by reference as he/she is creating the vector inside the function, you can't return references to local objects.
0

Featured Post

Do You Know the 4 Main Threat Actor Types?

Do you know the main threat actor types? Most attackers fall into one of four categories, each with their own favored tactics, techniques, and procedures.

Join & Write a Comment

Programmer's Notepad is, one of the best free text editing tools available, simply because the developers appear to have second-guessed every weird problem or issue a programmer is likely to run into. One of these problems is selecting and deleti…
This article shows you how to optimize memory allocations in C++ using placement new. Applicable especially to usecases dealing with creation of large number of objects. A brief on problem: Lets take example problem for simplicity: - I have a G…
The viewer will learn how to synchronize PHP projects with a remote server in NetBeans IDE 8.0 for Windows.
The goal of the video will be to teach the user the difference and consequence of passing data by value vs passing data by reference in C++. An example of passing data by value as well as an example of passing data by reference will be be given. Bot…

747 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

8 Experts available now in Live!

Get 1:1 Help Now