3

I have a Base class, with two derived classes DerivedA and DerivedB and a factory method that returns a pointer Base*.

Base* GetClass(int choice)
{
    if (choice == 0)
        return new DerivedA();
    else if (choice == 1)
        return new DerivedB();
    throw std::invalid_argument("no no no");
}

Both DerivedA and DerivedB implement a method, say GetDouble(), and this method is the only thing that interests me about constructing either of these classes:

double d = GetClass(0)->GetDouble();

My question is: when I call GetClass(0) I create a pointer that should be deleted. However, I am not instantiating it to an object. Should I still need to delete it?

Something like:

Base* c = GetClass(0);
double d = c->GetDouble();
delete c;

How would that work with std::unique_ptr?

Thank you in advance.

PS: I think there are related questions, like this and this, but mine is a little more specific.

Community
  • 1
  • 1
Girardi
  • 2,734
  • 3
  • 35
  • 50
  • What do you mean by *"I am not instantiating it to an object."*? Of course you're leaking memory (or other resources). – LogicStuff Aug 09 '16 at 16:42
  • 7
    Return a `std::unique_ptr` and you don't have trouble thinking about deleting it ;-) – Torbjörn Aug 09 '16 at 16:43
  • I mean as the last example that I gave, when I instantiate one of the class first to `Base* c`, then call `c->GetDouble()` and then delete `c` – Girardi Aug 09 '16 at 16:44
  • @Girardi: "Instantiating" is what you do to templates. The C++ description of the last example would be "Initialize the variable `Base *c`". – MSalters Aug 10 '16 at 09:44
  • @MSalters thanks for the info =) – Girardi Aug 10 '16 at 15:45

5 Answers5

8

By using std::unique_ptr, you wouldn't have the trouble of calling delete.

std::unique_ptr<Base> GetClass(int choice)
{
    if (choice == 0)
        return std::make_unique<DerivedA>();
    else if (choice == 1)
        return std::make_unique<DerivedB>();
    throw std::invalid_argument("no no no");
}

And you can now safely call:

double d = GetClass(0)->GetDouble();

What happens here is that: GetClass(0) will return an rvalue that will only be destroyed at the ;. Hence, chained calls such as GetClass(0)->func->func->etc(); will still have the object returned by GetClass() alive until ;

Remember ; is one of C++'s most powerful feature.

WhiZTiM
  • 21,207
  • 4
  • 43
  • 68
  • How would I go if I need to pass arguments to construct `DerivedA` and `DerivedB`? – Girardi Aug 09 '16 at 16:51
  • 1
    @Girardi Pass the arguments to `std::make_unique` as parameters for construction. – sjrowlinson Aug 09 '16 at 16:52
  • 1
    @Girardi, pass it as arguments to [`std::make_unique`](http://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique).. Example: `std::make_unique(arg1, arg2, arg3, ...etc);` – WhiZTiM Aug 09 '16 at 16:53
  • If you can avoid polymorphism completely, everything would be even better: std::variant GetClass(int choice) { if (choice == 0) return DerivedA {}; else if (choice == 1) return DerivedB {}; throw std::invalid_argument("no no no"); } – ebasconp Mar 18 '19 at 17:40
5

double d = GetClass(0)->GetDouble(); Is a memory leak. You new up the object in the function and then you return it. You use the returned object to call a member function and then you discard the pointer at the end of the expression. Since you no longer have the pointer you can longer call delete and free the memory that you grabed.

Remember every call to new/new[] needs a matching call to delete/delete[]

Now that we have smart pointers we can fix this using a std::unique_ptr. The pointer will manage the memory for you and once it goes out of scope it will delete the pointer.

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
3

You are in fact instantiating an object, but not storing a pointer to it, which is already a memory leak. Anytime you allocate an object with the use of new, it is your responsibility as a programmer to ensure is is deleted accordingly.

Your latter example works correctly. Using a std::unique_ptr will save you this worry as it automatically deletes its pointed-to object when it is destroyed.

alter_igel
  • 6,899
  • 3
  • 21
  • 40
2

Yes, you still need to delete it.

GetClass dynamically allocates storage for a DerivedA or DerivedB. Regardless of whether or not you store the resulting pointer anywhere, the memory will leak if it's not deleted.

As you note, std::unique_ptr<Base> solves your problem.

double d = GetClass(0)->GetDouble();

If GetClass returns a std::unique_ptr, then it will be destructed at the end of the full-expression containing it, which will delete the pointer.

TartanLlama
  • 63,752
  • 13
  • 157
  • 193
2

If you new you must delete (rare exceptions apply of course, like when you intentionally leak objects during shutdown of the program to speed it up and you know the destructors do nothing useful - but that's an edge case). Simple as that. It does not matter if you assign the return value of new to a variable or not - memory is still allocated and needs to be released. If you don't keep track of the returned pointer to delete it later it is simply leaked.

Using std::unique_ptr helps you by taking ownership of the pointer and releasing it when the unique_ptr goes out of scope.

Jesper Juhl
  • 30,449
  • 3
  • 47
  • 70