[Okta Webinar] Learn how to a build a cloud-first strategyRegister Now

x
?
Solved

memory allocation/deallocation for a vector of pointers

Posted on 2004-09-02
16
Medium Priority
?
965 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
13 Comments
 
LVL 11

Accepted Solution

by:
avizit earned 400 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
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 30

Assisted Solution

by:Axter
Axter earned 400 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
 
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 400 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 400 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

Free Tool: Port Scanner

Check which ports are open to the outside world. Helps make sure that your firewall rules are working as intended.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Many modern programming languages support the concept of a property -- a class member that combines characteristics of both a data member and a method.  These are sometimes called "smart fields" because you can add logic that is applied automaticall…
Article by: evilrix
Looking for a way to avoid searching through large data sets for data that doesn't exist? A Bloom Filter might be what you need. This data structure is a probabilistic filter that allows you to avoid unnecessary searches when you know the data defin…
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.
The viewer will learn how to clear a vector as well as how to detect empty vectors in C++.
Suggested Courses

873 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