What stupid thing am I doing here with unique_ptr?

General JUCE discussion
1

I’m running into some kind of issue with a mis-managed pointer. Here is a bit of test code, simplified from my plug-in, which demonstrates the problem.

I’m using a couple of unique_ptrs here to parse some XML. The method seems to exit OK, but then throws a EXC_BAD_ACCESS error.

void loadFirstPreset()
{
    String examplePreset = "<Factory_Presets><PRESET name='my fav preset'><PARAM id='decay' value='1'/><PARAM id='filter' value='1'/><PARAM id='mix' value='1'/><PARAM id='dist' value='6.375251412391663e-1'/><PARAM id='stereo' value='1'/></PRESET></Factory_Presets>";
    
    std::unique_ptr<XmlElement> xml (parseXML(examplePreset));
    
    if (xml.get() != nullptr)  // check if valid xml
    {
        DBG ("XML is valid");
        std::unique_ptr<XmlElement> presetXml (xml->getFirstChildElement());
        if (presetXml.get() != nullptr)
        {
            DBG ("Parameter count: " + (String)presetXml->getNumChildElements());
            DBG (presetXml->createDocument(String(), false, false));
        }
        else DBG ("Preset not found");
    }
    else
    {
        DBG ("String is not valid XML");
    }
}

Output to the console reads as it should:

XML is valid
Parameter count: 5
<PRESET name="my fav preset">
  <PARAM id="decay" value="1"/>
  <PARAM id="filter" value="1"/>
  <PARAM id="mix" value="1"/>
  <PARAM id="dist" value="6.375251412391663e-1"/>
  <PARAM id="stereo" value="1"/>
</PRESET>

But the app (a standalone of a plug-in) crashes, and Xcode shows this error:

JUCE Message Thread (1): EXC_BAD_ACCESS (code=EXC_I386_GPFLT)

Next to juce_LinkedListPointer.h, line 98, which is the operator in that class which “Returns the item which this pointer points to”.

So I expect I’m doing something I shouldn’t with those unique_ptrs, but trying different variations hasn’t gotten me anywhere. Please help a dummy out!

2 
    std::unique_ptr<XmlElement> presetXml (xml->getFirstChildElement());

This line creates a unique_ptr that owns the child element, when it goes out of scope (end of the if body) the child element will be deleted. However when the parent (xml) goes out of scope at the end of the function it tries to delete all its children.

The solution is to not use a unique_ptr in there, just

auto presetXml = xml->getFirstChildElement(); 
if (presetXml)
{ /* etc */ }
1 Like
3

If only people would use ‘auto’, this kind of problem would go away!

Instead of verbose code like

    std::unique_ptr<XmlElement> xml (parseXML(examplePreset));
    
    if (xml.get() != nullptr)  // check if valid xml
    {
        DBG ("XML is valid");
        std::unique_ptr<XmlElement> presetXml (xml->getFirstChildElement());
        if (presetXml.get() != nullptr)
        {

all you need to write is this:

    if (auto xml = parseXML(examplePreset))
    {
        DBG ("XML is valid");

        if (auto presetXml = xml->getFirstChildElement())
        {

…and not only would the ownership be correct, but you’d have much more shorter, more readable code, and better scope hygiene.

I’m constantly surprised by how much people seem to over-complicate the way they write expressions!

3 Likes
4

Thanks for the explanation! I’ll have to dig more into how a unique_ptr manages the parent/child relationship to fully understand the blunder, but this gets me on the right track for now.

5

It does look more readable, that’s for sure.

I did a lot of C++ learning using the JUCE tutorials, and since they explicitly declare unique_ptrs for XmlElements, I made a habit out of that.

Also, while learning, I thought it would be better to explicitly declare in general, to avoid lazy thinking about variable typing. But this is one instance where the “better scope hygiene” would have helped me, if I had trusted the benevolent judgement of the auto oracle to pick the right type for me!

6

Ah, some of our tutorials probably predate C++11 support! We should tidy those up to make sure we’re setting a good example!