New flag to help avoid smart-pointer accidents: JUCE_STRICT_REFCOUNTEDPOINTER


#1

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:

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 :slight_smile:


#2

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


#3

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…