OwnedArray and pointer safety

My grasp of pointers is quite weak, and I’m hoping someone more knowledgeable can tell me if I am using OwnedArray in a safe way.

What I’m doing is adding components (in this case Label) to an OwnedArray to make it easy to iterate over them later on. So I create each Label, add it to an OwnedArray, and do some setup on it. Here’s a stripped down example:

OwnedArray<Label> myOwnedArray;
Label* myLabel = new Label("label-name", "Some Text");
myOwnedArray.add(myLabel);
// Do some setup on myLabel, e.g.
addAndMakeVisible(myLabel);

I’ve been reading about how raw pointers and “new” should be avoided in favour of smart pointers, and it’s made me worried that my approach is not ideal. I think my approach is safe, since OwnedArray takes ownership of the raw myLabel pointer?

Nevertheless I have also tried a different approach, passing a smart pointer to the OwnedArray’s add method:

OwnedArray<Label> myOwnedArray;
auto myLabel = myOwnedArray.add(std::make_unique<Label>("label-name", "Some Text"));
// Do some setup on myLabel, e.g.
addAndMakeVisible(myLabel);

Is one approach preferable to the other? Or is there a better approach altogether?

1 Like

In your use case it doesn’t really matter if you use new or make_unique. (Ignoring here any possible C++ exceptions stuff but you are not catching the exceptions anyway in your current code.)

1 Like

Thanks. I’m glad to hear that I’m not doing it completely wrong!

The code you’ve written looks fine, but if you’re new to smart pointers, it’s probably worth thinking about some of the reasons why you should get in the habit of using std::make_unique, rather than new.

Your code allows you to do something silly like return myLabel. (You don’t intend to do this right now of course, but will you remember not to do it when you make changes to your code in 6 months time?) If you did return the pointer, then you could end up giving it to some other object that has no idea about the lifetime of the object. There would then be nothing stopping it from trying to dereference a deleted object and crashing. On the other hand, if you were to return a smart pointer to the object, its lifetime would be automatically managed.

Secondly, suppose that an error is thrown after you create the new object but before you’ve handed it to the OwnedArray. (Again, it’s not going to happen in the code you’ve written, but suppose you come back in a year and add the line of code myLabel->doSomethingRisky() on the third line). If an error is thrown, the stack will unwind, and when it passes the current scope, you’ll be left with a memory leak. But this can’t happen if you’re using std::make_unique, as the destructor will automatically delete the object.

Personally, I allow myself to use new only when I’m placing it directly into the (smart) container, so that I know it can’t be exposed.

 myOwnedArray.add(new Label("label-name", "Some Text"));

All other times, I try to make sure that I’m using smart pointers.

Regarding exceptions, I would mostly ignore anything to do with them in Juce based code. They can only be sensibly dealt with in Juce stand alone applications by setting the exception handler function but in plugins, as far as I know, you would need to add try catch blocks everywhere where an exception might happen. I have myself usually opted for “if a C++ exception happens, just let the code crash” but if there actually is a way to add a centralized exception handler for plugins too, I’d be interested to know how that can be done…

yes, but what @LiamG suggest is just good practice. or, said another way, bad practice in places where it doesn’t matter encourages bad practices… :wink:

2 Likes

If it makes sense in the context of your application I’d move the ownership transfer at the end like that

// First create it
auto myLabel = std::make_unique<Label> ("label-name", "Some Text");

// Work on it
addAndMakeVisible (*myLabel);
myLabel->setSomething();

// Transfer ownership
myOwnedArray.add (std::move (myLabel));

// Note: Down here myLabel is no longer valid as it has been moved

When code gets more complex, this e.g. allows you to safely do something like that:

auto myLabel = std::make_unique<Label> ("label-name", "Some Text");

// some block of code working with myLabel

if (someCondition)
    return; // myLabel will safely be deleted here

myOwnedArray.add (std::move (myLabel));
1 Like

The reasons you’ve listed are enough to convince me that smart pointers are the better option. Thanks for spelling out the benefits :slight_smile:

I was actually struggling to figure out how to create the smart pointer outside of add(), then add it to the OwnedArray later on. std::move works great!

Another option for this is myOwnedArray.add(myLabel.release()). The std::unique_ptr::release() function returns the raw pointer and then nullifies the unique_ptr. (If you look inside the OwnedArray::add(std::unique_ptr) function you’ll see that it’s only calling release() anyway). You just need to make sure that you don’t call anything on the unique_ptr after release(), or else you’ll get a runtime error.

Here is another option which avoids having any exposed pointers, smart or raw:

OwnedArray<Label> myOwnedArray;
myOwnedArray.add(new Label("label-name", "Some Text"));
addAndMakeVisible(myOwnedArray.getLast());
1 Like

Good to know. I’m learning a lot about smart pointers today!

You can use make_unique to avoid using new there, and since OwnedArray::add returns a pointer to the element that was added you can just do:

OwnedArray<Label> myOwnedArray;
addAndMakeVisible (myOwnedArray.add (std::make_unique<Label> ("Name", "Text")));