New flag to help avoid smart-pointer accidents: JUCE_STRICT_REFCOUNTEDPOINTER

[jules]

The old ReferenceCountedObjectPtr class is pretty ancient, and when I wrote it, I made it pretty lenient in terms of what it allows you to do with it - in particular, it has a cast operator that allows it to be implicitly cast into a raw pointer to the underlying object.

Now, that may seem like a pretty sensible thing to do, as it means you can pretty much mix smart-pointers with raw pointers with very little syntax. However, having spotted a lot of places where raw and smart pointers are being carelessly returned and silently converted from one type to another, it’s clear that people just can’t be trusted to use it safely and efficiently (myself included!)

So what I’ve just added is a new flag JUCE_STRICT_REFCOUNTEDPOINTER, which removes this implicit cast:

Added flag JUCE_STRICT_REFCOUNTEDPOINTER which is recommended to avoid accidental leaks when using ref-counted pointers. Enabled this flag in the demo projects, and used it to help tidy up some dubious smart-pointer use

By default, the flag is disabled, because it’ll break a lot of code, so for the moment it’s opt-in. But I do recommend that people try turning it on in their projects, and see what it breaks in their codebase.

What you’ll probably find is that there are lots of places where you have functions which e.g. return a raw or smart pointer, which then gets stored in the opposite type of pointer. This is the kind of thing that in many cases you’ll get away with, but then you’ll find edge-cases where an object only has a single ref count, so it gets unexpectedly deleted during one of these conversions…
You may also have places where you pass a smart pointer into a function that takes a raw pointer, when actually it’d be better for that function to take a reference or smart pointer instead. I also found a lot of constructors that took a raw pointer, but then stored it in a smart pointer member, where it’d be better to simply have the constructor take a smart pointer and std::move it into the member variable.

With the new flag on, all of these cases will fail to compile, forcing you to think harder about what you’re actually trying to do, and either make a better decision about when raw or smart pointers should be used, or add an explicit call to .get() to extract the raw pointer if that’s really the right thing to do.

Hope it’s useful and saves a few people from obscure bugs

[railjonrogut]

I ran into an issue with my old code and the AudioProcessorGraph…

With variable:

 AudioProcessorGraph::Node::Ptr trimNode

I had:

 pGraph->removeNode (trimNode)

and since the change to ReferenceCountedObjectPtr this calls

bool removeNode (NodeID);

instead of

 bool removeNode (Node*);

The latter probably needs to be changed to

 bool removeNode (Node::Ptr);

I didn’t get any errors or warnings with the JUCE_STRICT_REFCOUNTEDPOINTER flag enabled.

I’ve just made sure all my calls to removeNode() are to removeNode (NodeID) directly.

 pGraph->removeNode (trimNode->nodeID)

Cheers,

Rail

[jules]

That’s very surprising… it must be deciding to silently cast the Ptr to a bool and then to an integer! Nasty… I don’t think changing the parameter type would help prevent other surprising casts, I should probably make the NodeID into a stronger type. Will push something shortly…

[graeme-2]

Hi Jules, I’ve just updated from prior to this change to latest master and have encountered an issue with this (or some related change to ref object/ptr) in my code. I’ve verified the issue with the following contrived test done in the Projucer code on latest develop:

If JUCE_STRICT_REFCOUNTEDPOINTER is turned on then no problem.
But if it’s off, then the following occurs:

    //contrived example using a random juce ref counted object
    ReferenceCountedObjectPtr<ImagePixelData> upPtr;
    ImagePixelData * up = nullptr;
    
    if (upPtr == up) //compile fail
        ....

//Reasons:
//Use of overloaded operator '==' is ambiguous (with operand types 'ReferenceCountedObjectPtr<juce::ImagePixelData>' and 'juce::ImagePixelData *')
//juce_ReferenceCountedObject.h:404:10: Candidate function
//juce_ReferenceCountedObject.h:406:10: Candidate function
//Built-in candidate operator==(class juce::ImagePixelData *, class juce::ImagePixelData *)
//Built-in candidate operator==(const class juce::ImagePixelData *, const class juce::ImagePixelData *)
//Built-in candidate operator==(volatile class juce::ImagePixelData *, volatile class juce::ImagePixelData *)
//Built-in candidate operator==(const volatile class juce::ImagePixelData *, const volatile class juce::ImagePixelData *)

Edit: I should mention that this is on macOs High Sierra with Xcode 9.4.1.

[mcmartin]

From juce_ReferenceCountedObject.h:

   #if JUCE_STRICT_REFCOUNTEDPOINTER
    /** Checks whether this pointer is null */
    explicit operator bool() const noexcept                 { return referencedObject != nullptr; }

   #else
    /** Returns the object that this pointer references.
        The pointer returned may be null, of course.
        Note that this methods allows the compiler to be very lenient with what it allows you to do
        with the pointer, it's safer to disable this by setting JUCE_STRICT_REFCOUNTEDPOINTER=1, which
        increased type safety and can prevent some common slip-ups.
    */
    operator ReferencedType*() const noexcept               { return referencedObject; }
   #endif

This means that you should write if (!upPtr) instead of if (upPtr == nullptr).

[graeme-2]

Thanks McMartin. My example was actually just meant to recreate an equality check between a ReferenceCountedObjectPtr and a raw pointer. So nothing nullptr specific.

[mcmartin]

I actually missed something in the source code, so what I said in my previous post is wrong

    /** Checks whether this pointer is null */
    bool operator== (decltype (nullptr)) const noexcept     { return referencedObject == nullptr; }
    /** Checks whether this pointer is null */
    bool operator!= (decltype (nullptr)) const noexcept     { return referencedObject != nullptr; }

It is totally fine to compare a ReferenceCountedObjectPtr with nullptr.

Back to your original problem, I guess the only solution (with the current state of the code) is to write if (upPtr.get() == up).

[graeme-2]

No prob. Yeah I could do the .get(), but that would mean a good number of changes and it’s a bit of a questionable effort seeing as strict mode allows it as is. I’ll hope for a fix, but I can probably swap out the ref object code with a previous version in my own fork if needed too. (Edit: nope, not reasonable to revert ref object back. 1e6bbb8 commit has many changes relying on the newer code. )

[graeme-2]

Bump. Coming back to look at this again. It’s keeping me from being able to stay up to date with master so if someone can look into it I’d appreciate it.

To reiterate, the issue is that with strict mode off, code that compares a ref ptr with a raw pointer no longer compiles. It does however work when strict mode is turned on.

I’ve created 2 pips to demonstrate the issue. One has strict mode off and the other has it on.

Strict Ref Off Console.h (1.2 KB) Strict Ref On Console.h (1.0 KB)

My hope is that this can be fixed to allow these comparisons to continue to work with strict mode off.

thanks