Getting rid of ScopedPointer

General JUCE discussion
1

Hi, I’ve started getting rid of this, but am having a particular issue. This was my old code:

MidiFile mf;
if ( ScopedPointer<FileOutputStream> p_os = outf.createOutputStream() )
    mf.writeTo( *p_os, 0 );

Which I’m trying to replace with (I think):

if ( auto p_os = std::make_unique<FileOutputStream *>( outf.createOutputStream() ) ) 
    mf.writeTo( **p_os.get(), 0 );

Which just seems wrong a very ugly. Is there a better way of dealing with this?

thx

3

You won’t need std::make_unique in this case. You use make_unique to invoke an object’s constructor. In your case you let File::createOutputStream() do the work of constructing the object as it returns a pointer to a new instance of the output stream and now you are responsible to take over the ownership.

To do this with a unique ptr you can just pass in the raw pointer to the unique ptr’s constructor or reassign the raw pointer to the unique ptr via reset. So it would be:

if ( std::unique_ptr<FileOutputStream> p_os ( outf.createOutputStream())) 
    mf.writeTo ( *p_os.get(), 0 );

What you did above was creating a unique ptr that does not hold the FileOutputStream object itself but a raw pointer to a FileOutputStream. When your unique ptr goes out of scope it will delete the raw pointer it holds but not the actual instance the pointer points to and in the end you’ll leak memory

4

hi, thx for speedy reply. that makes sense.

That code won’t compile for me tho, I have to go back to old style:

std::unique_ptr<FileOutputStream> p_os( outf.createOutputStream() );

if ( p_os.get() )
    mf.writeTo( *p_os.get(), 0 );

which seems like a bit of a step backwards?

5 
if (auto p_os = std::unique_ptr<FileOutputStream> (outf.createOutputStream())) 
    mf.writeTo (*p_os, 0);
1 Like
6

I guess this is just ugly during the transisiton. JUCE started to change the interface in other places, I would hope this will follow:

File::createOutputStream() should return a std::unique_ptr<FileOutputStream> in the first place.

But @dave96’s version doesn’t look that bad either

1 Like
7

thx. that’s better to my eyes.

8

Yeah, sorry for the quick reply. Returning by std::unique_ptr is definitely the best way to do this, I was just trying to illustrate how to scope correctly in if statements if you are dealing with legacy APIs.

9

Oh wait you are right, that was a bit quick. @dave96’s is the correct way. Side note, the JUCE team announced to update the API so that functions like that will return unique ptrs instead of raw pointers and the code might then read like

if (auto p_os = outf.createOutputStream()) 
    mf.writeTo (*p_os, 0);
10

I think that we shouldn’t expect a change in the interface here: the docs discourage the use of File::createOutputStream() in new code –

Note that this is an old method, and actually it’s usually best to avoid it and instead use an RAII pattern with an FileOutputStream directly, e.g.

(etc.)

1 Like
11

Good point, this can still be well scoped with C++17’s “if initialisers”.

2 Likes
12

ah, thx.

13

If you do have to deal with legacy APIs, a nice helper function is this:

template <typename T>
std::unique_ptr<T> rawToUnique (T* t)
{
    return std::unique_ptr<T> { t };
}

It automatically infers the type of the pointer, so that you don’t have to explicitly mention it (i.e. there’s fewer chances to make type conversion errors). It would simplify this situation like so:

if (auto ptr = rawToUnique (outf.createOutputStream()))
    mv.writeTo (*ptr, 0);

The only gotcha is that if you accidentally pass a pointer-to-array instead of a pointer-to-element, the unique_ptr destructor will call delete instead of delete[], but this is a problem with ScopedPointer too afaik.

2 Likes
14

Come to think of it, a better name for rawToUnique might be cookPtr seeing as

  • it’s shorter, and
  • after the function call, the pointer is no longer raw
10 Likes
15

lol

16

For old code that used to do things like:

        addAndMakeVisible( m_label = new MyLabel( "Label" ) );

is this the best way of going about replacing it?

        m_label.reset( new MyLabel( "Label" ) );
        addAndMakeVisible( m_label.get() );
17 
m_label = std::make_unique<MyLabel> ("Label");
addAndMakeVisible (*m_label);
1 Like
18

Change the MyLabel to be on the stack instead of the heap.

Rail

2 Likes
19

Can’t in this case, but in all my old Projucer generated code I am doing.

20

I can’t say I enjoyed replacing ScopedPointer with std::unique_ptr, it is more verbose, and the error messages of the compiler are longer and more complicated to read when you use std::make_unique. Changing existing code that uses ScopedPointer with std::unique_ptr stuff requires a lot of small changes, and at the end the code is just longer and uglier, I feel. So after replacing half of the ScopedPointers in my code with std::unique_ptr, I rolled back everything and decided to use something that is between them, and requires just a simple search and replace for “ScopedPointer”:

/**
   a std::unique_ptr<T> enriched with an implicit T* conversion , works as a convenient replacement
   for the deprecated juce ScopedPointer.
*/
template <typename T>
class Owned : public std::unique_ptr<T> {
public:
  Owned() {}
  Owned(T *p) : std::unique_ptr<T>(p) {}
  operator T*() const { return this->get(); }
};
3 Likes
21

I definitely agree with the longer and uglier aspect. It just feels clunky.

1 Like