1

How do I delete the space I allocated with new when I need to return the result. I used this so if I don't find an object I return back an object with a dummy sentinel value.

ClassObject* ClassObjectTwo::find(string findid) {
        ClassObject *sendback;
        bool found;

        for(vector<ClassObject>::iterator it = data.begin(); it != data.end(); it++) {
                if ( it->get_id() == findid) {
                        found = true;
                        sendback = &(*it);
                }
        }

        if(!found) {
                sendback = new ClassObject;
                sendback->set_key(100);
        }

        return sendback;
}

Or is this not a problem as its destroyed when out of scope. The only other solution I can think of is place the object in the constructor and delete via the constructor. I just didn't want to add a variable for one function.

Improve this question
7
  • 1
    Use the delete operator to get rid of objects allocated with new. Commented May 24, 2013 at 21:15
  • 1
    if you don't find it, return NULL. Commented May 24, 2013 at 21:16
  • @Ryan Generally speaking, if you allocate something with new it won't go "out of scope" and won't be deallocated for you automatically. You must deallocate it using delete. (Pedantry: it is possible that some classes are coded so as to allow the object to automatically delete itself - or be deallocated without you explicitly calling delete but this requires extra code by the programmer and isn't normally a consideration). Commented May 24, 2013 at 21:21
  • 1
    Returning NULL is best. You could investigate auto_ptr or unique_ptr. Commented May 24, 2013 at 21:24
  • 1
    @danf auto_ptr is obsolete and should not be used any more. Commented May 24, 2013 at 22:07

4 Answers 4

Reset to default
6

There is some great opportunity to learn here, so I hope that you don't mind me straying a bit from your question. First of all you should get in the habit of never working with owning raw pointers, i.e. don't allocate an object and hold it in a raw pointer. A raw pointer is for instance the variable sendback in your function which is of type ClassObject * . By "owning" I mean the fact that you are responsible for deleting the memory yourself.

Instead, you should always keep the ownership of the object in a smart pointer if it needs to be dynamically allocated. I.e. std::unique_ptr or std::shared_ptr with preference for unique_ptr unless you really need shared ownership. If you don't have a C++11 compatible compiler you can use the Boost equivalents boost::scoped_ptr and boost::shared_ptr. So whenever you want to dynamically allocate an object, always place it in a smart pointer - this will ensure that the object is destroyed when the smart pointer goes out of scope. Now if you want to return a dynamically created object, return a smart pointer that holds the object. As a rule of thumb, if you need to write delete in your code, you probably should have used a smart pointer.

For instance, to create a new dynamically created ClassObject, you would write:

std::unique_ptr<ClassObject> myObj = std::unique_ptr<ClassObject>(new ClassObject());

or even better, use auto to avoid having to write the type explicitely:

auto myObj = std::unique_ptr<ClassObject>(new ClassObject());

with c++14 that should become:

auto myObj = std::make_unique<ClassObject>();

Using a smart pointer prevents you from running into some potential problems with making sure that these objects get deleted in the face of exceptions.

In your case however, you don't really need to return an object at all. If you want to indicate that there was no object found, then return NULL (or really you should use the C++11 nullptr if it is available with your compiler). Another option is to return the iterator to the object you found or data.end() if you couldn't find it.

You also need to be very careful when using your function, since the user of the function does not have any guarantee that the object that you return a pointer to isn't destroyed by some subsequent operation - but this may be more of a documentation issue.

Another point to make might be to take the argument to the function as a const string & instead of just a string to avoid a temporary copy of the parameter (that might happen).

There is also some more discussion on a question remarkably similar to your use case if you take a look at Herb Sutter's blog:

http://herbsutter.com/2013/05/13/gotw-2-solution-temporary-objects/

as well as some further discussion on this subject at:

http://herbsutter.com/2013/05/16/gotw-3-solution-using-the-standard-library-or-temporaries-revisited/

Herb Sutter has written a number of excellent books on C++ and is taking active part in the further development of the C++ language, so don't take my word for this, take his... :).

Cheers!

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

This is a good explanation and thank you very much for the detailed response!
5

It is perfectly fine to destroy a dynamically created object outside the function.

However, in this case it would be better to return NULL.

Improve this answer

2 Comments

I would argue that while it is perfectly legal to return a dynamically allocated object and let the caller delete it, it is however not "perfectly fine" :) - it is error prone and places an unecessary burden on the caller of the function.
Setting it to NULL instead of a junk object is way cleaner!
0

When you assign the object to a pointer outside the function, you can delete from there to avoid memory leak. I understand you need the object pointer from the function, and therefore deleting outside the function seems to make sense most to me. You delete after using it. if it returns null, then you don't have to delete anything.

Improve this answer

Comments

0

I do not understand why you create an object if you return the object. It doesn't make sense in my opinion. The caller should check the return value and if it's null do something, like creating a dummy object. You're method is called find and that tells me it's contract is to search for an item and return it or return a value that indicates failure (nullptr). If you want to create an object in case the searched object is not found, then call it first_or_default or something similar, since this is what it actually does.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.