• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 970
  • Last Modified:

memory allocation/deallocation for a vector of pointers

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
axmeno
Asked:
axmeno
  • 5
  • 3
  • 2
  • +3
4 Solutions
 
avizitCommented:
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
 
axmenoAuthor Commented:
No dice.  This seems to do the same thing -- same memory usage and same error message.
0
 
axmenoAuthor Commented:
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
Upgrade your Question Security!

Your question, your audience. Choose who sees your identity—and your question—with question security.

 
AxterCommented:
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
 
avizitCommented:
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
 
axmenoAuthor Commented:
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
 
AxterCommented:
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
 
AxterCommented:
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
 
axmenoAuthor Commented:
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
 
axmenoAuthor Commented:
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
 
IndrawatiCommented:
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
 
teratomaCommented:
If you used boost::shared_pointer (www.boost.org) you wouldn't have to delete anything.
0
 
s_senthil_kumarCommented:
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
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

Featured Post

Cloud Class® Course: MCSA MCSE Windows Server 2012

This course teaches how to install and configure Windows Server 2012 R2.  It is the first step on your path to becoming a Microsoft Certified Solutions Expert (MCSE).

  • 5
  • 3
  • 2
  • +3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now