?
Solved

memory allocation/deallocation for a vector of pointers

Posted on 2004-09-02
16
Medium Priority
?
963 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 5
  • 3
  • 2
  • +3
16 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

VIDEO: THE CONCERTO CLOUD FOR HEALTHCARE

Modern healthcare requires a modern cloud. View this brief video to understand how the Concerto Cloud for Healthcare can help your organization.

Question has a verified solution.

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

Introduction This article is a continuation of the C/C++ Visual Studio Express debugger series. Part 1 provided a quick start guide in using the debugger. Part 2 focused on additional topics in breakpoints. As your assignments become a little more …
Go is an acronym of golang, is a programming language developed Google in 2007. Go is a new language that is mostly in the C family, with significant input from Pascal/Modula/Oberon family. Hence Go arisen as low-level language with fast compilation…
The viewer will learn how to use and create new code templates in NetBeans IDE 8.0 for Windows.
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.
Suggested Courses

770 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