Sign in to follow this  

allocating new memory in a function

This topic is 3592 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

my goal is to create a function that takes an existing container as an argument and places an object in it.

class foo { 
private:
   int kill;
public:
   foo() : kill(0)
   int get_kill() {return kill;} 
   void set_kill(new_kill) {kill = new_kill;}   
};

void create_foo (vector<foo*> & foo_list)
{
   foo * object = new foo;
   foo_list.push_back(&(*object));
}

void destroy_foo (vector<foo*> & foo_list)
{
   foo_list::iterator it;
   
   for (int i = 0, end = foo_list.end(); i < end; ++i, end = foo_list.end()){
      if (foo_list[i]->get_kill()) {
         it = foo_list.begin() + i;
         foo_list.erase(it);
      }
   }   
}      
int main()
{
   vector<foo*> foo_list;   
   create_foo(foo_list);
   foo_list[0]->set_kill(1);
   destroy_foo(foo_list);
}


so i'm wondering if i will get a memory leak since I use new but I don't use delete. I would figure that the pointer would die at the end of the function block, and that the memory at the address would be wiped because of the erase method. It almost looks like I dont need a delete here, but i know there must be something evil lurking here. If there is a memory leak how would i fix it. I can't make a function that goes "delete object" because object doesn't exist anymore.

Share this post


Link to post
Share on other sites
It's a leak. If there's a new, there must be a matching delete. Always.

Just delete all the elements of the vector<foo*> you created before it gets destroyed.

By the way, there's no need to erase each element of the list. The vector destructor will take care of that for you when the vector goes out of scope. What it WON'T do is deallocate the memory its elements points to, if they're pointers.

Edit: Oh, also, if you just want to clear the vector, there's a much easier and faster way than erasing each element: Call the clear() method. :)

Share this post


Link to post
Share on other sites
On topic:
1. Smart pointers.
2. If you really can't use smart pointers, destroy_foo() should delete the object (*it).

Off topic:
1. This is redundant:
it = foo_list.begin() + i;
foo_list.erase(it);

You could just as well do:
it = foo_list.erase(it);
And that will work for other STL containers like std::list, where you can't get random access to elements through iterators (foo_list.begin()+i will give a compile error).

2. The whole create_foo() and destroy_foo() stuff isn't particularly OOPy, I'd be inclined to make foo_list a member variable and create it / destroy it in member functions, or the constructor / destructor where possible.

EDIT: In fact, the whole erase loop looks a bit dodgey. I'd make it:

for(foo_list::iterator it=foo_list.begin(); it!=foo_list.end();)
{
if((*it)->get_kill())
{
delete *it; // If you're not using smart pointers
it = foo_list.erase(it);
}
else
++it;
}



EDIT #2: What's going on here? foo_list.push_back(&(*object));
What's wrong with: foo_list.push_back(object); ?

Share this post


Link to post
Share on other sites
Quote:
Original post by caleb_yau

class foo {
private:
int kill;
public:
foo() : kill(0)
int get_kill() {return kill;}
void set_kill(new_kill) {kill = new_kill;}
};


You mean:

// ...
foo() : kill(0) { }
// ...
void set_kill(int new_kill) { kill = new_kill; }

I assume?


void create_foo (vector<foo*> & foo_list)
{
foo * object = new foo;
foo_list.push_back(&(*object));
}

This function has a memory leak if push_back throws an exception (it can throw an std::bad_alloc if your out of memory) because object won't get deleted in this scenario.

You can also call push_back(object) rather than push_back(&(*object)).


void destroy_foo (vector<foo*> & foo_list)
{
foo_list::iterator it;

for (int i = 0, end = foo_list.end(); i < end; ++i, end = foo_list.end()){
if (foo_list[i]->get_kill()) {
it = foo_list.begin() + i;
foo_list.erase(it);
}
}
}

Yes, more leakage here I'm afraid.

Here's one solution that adapts your existing code:

struct deleteit
{
template<typename T> void operator() (T *p) const { delete p; }
};

void destroy_foo (vector<foo*> & foo_list)
{
foo_list::iterator rem_begin =
std::remove_if( foo_list.begin(), foo_list.end(),
std::mem_fun(&foo::get_kill) );

std::for_each(rem_begin, foo_list.end(), deleteit());
foo_list.erase(rem_begin, foo_list.end());
}


Quote:
If there is a memory leak how would i fix it. I can't make a function that goes "delete object" because object doesn't exist anymore.


I've suggested one way. It's not the best, however. Here are some other suggestions in order of preference.

Solution 1: Don't use pointers! Why not just std::vector<foo>?

Solution 2: Use a boost::ptr_vector<foo> if you really need pointers.

Solution 3: Consider std::vector<boost::shared_ptr<foo> > if you need to have multiple foo references floating about your code.

All three of these solutions make the code exception safe more-or-less automatically and do the deletion for you.

As a rule of thumb, if you find that you're calling delete manually you could well be doing something wrong.

EDIT: on reflection you appear to have the whole "kill" thing to indicate which elements need removal. This is a very fishy design. To kill something just remove it from the container rather than going round the houses.

Share this post


Link to post
Share on other sites
Quote:
Original post by the_edd
Solution 1: Don't use pointers! Why not just std::vector<foo>?

Solution 2: Use a boost::ptr_vector<foo> if you really need pointers.

Solution 3: Consider std::vector<boost::shared_ptr<foo> > if you need to have multiple foo references floating about your code.


Quoted for extreme emphasis.

Quote:
EDIT: on reflection you appear to have the whole "kill" thing to indicate which elements need removal. This is a very fishy design. To kill something just remove it from the container rather than going round the houses.


No, there are good reasons for this. However, there's also a much saner way to go about the removal.


#include <vector>
#include <algorithm>

struct foo {
// If you are going to just offer direct access to the member, then don't
// pretend it's private. Also, C++ has a real boolean type; use it.
bool kill;
foo(): kill(false) {}
};

// We need a little adaptor here:
bool dead(const foo& f) { return f.kill; }

void add_foo(std::vector<foo>& foos) {
foos.push_back(foo()); // yes, it's that easy.
}

void remove_dead_foos(std::vector<foo>& foos) {
foos.erase(std::remove_if(foos.begin(), foos.end(), dead), foos.end());
} // Again, yes, it's that easy. Thanks to <algorithm> :)


int main() {
vector<foo> foo_list;
add_foo(foo_list);
foo_list[0].kill = true;
remove_dead_foos(foo_list);
}



If you do need indirection (because foos are shared and/or polymorphic), then using smart pointers (or a ptr_container, if they are not shared) will let you do things basically the same way: the automatic management of the pointers will prevent you from having to mess around with new and delete. Life is a lot easier and more fun this way :)

Also, what's up with three space indent?

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Also, what's up with three space indent?


That's a weird complaint. Especially from someone who's programmed with Python.

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
Quote:
Original post by Zahlman
Also, what's up with three space indent?


That's a weird complaint. Especially from someone who's programmed with Python.


I use tabs in Python. :) But they're not very nice for forum posting unless you're copying and pasting from somewhere.

I dunno. It just seems strange to have an odd number of spaces for the indent. But I have seen a fair number people using 3 spaces, actually.

In any case, it beats the hell out of the 4 space, tab, tab + 4 space, 2 tabs etc. pattern. Whoever even made it an option for whatever IDE it is that does that, should be fired.

Share this post


Link to post
Share on other sites

This topic is 3592 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this