1

I will load multiple XML files, each into a QDomDocument, and then use a QMap to associate an identifying string with each document. Should I store a QDomDocument or a pointer to a QDomDocument in the map? i.e., which of the following examples is more in keeping with Qt best design practices.

I suspect that example A is preferred. All the code samples I see just create a local QDomDocument on the stack. And, sizeof( QDomDocument ) is 4 bytes; so, QDomDocument is probably a thin wrapper that can be shallow copied without much of a performance hit.

A: Map contains QDomDocument instances

class Core
{
private:
  QMap<QString, QDomDocument> docs;

public:
  Core( void )
  {
    QFile file( "alpha.xml" );
    file.open( QIODevice::ReadOnly );

    QDomDocument doc;
    doc.setContent( &file );
    docs["alpha"] = doc;

    // ... etc for other XML files
  }

  QString findThing( QString const & docName, QString const & thingName )
  {
    QDomDocument doc = docs[docName];

    // ... search the doc for the thing with the given name
  }
};

B. Map contains pointers to QDomDocument instances

class Core
{
private:
  QMap<QString, QDomDocument *> docs;

public:
  Core( void )
  {
    QFile file( "alpha.xml" );
    file.open( QIODevice::ReadOnly );

    QDomDocument * pDoc = new QDomDocument();
    pDoc->setContent( &file );
    docs["alpha"] = pDoc;

    // ... etc for other XML files
  }

  QString findThing( QString const & docName, QString const & thingName )
  {
    QDomDocument * pDoc = docs[docName];

    // ... search the doc for the thing with the given name
  }
};
Mike Finch
  • 746
  • 1
  • 7
  • 20
  • `QDomDocument * pDoc = new QDomDocument();` -- Did you take in consideration the cost of calling the memory allocator each time? – PaulMcKenzie Sep 25 '19 at 14:50

2 Answers2

3

The OP suspicions are quite right: QDomDocument holds a pointer to its implementation (PIMPL) through its base class, QDomNode.

While fonZ is right in saying the original object will go out of scope and destroyed, the copy stored in the map will keep the (shared) implementation alive. Taking a look at the source, we see an empty destructor for QDomDocument, and its base class destructor reveals a reference counting mechanism:

QDomNode::~QDomNode()
{
    if (impl && !impl->ref.deref())
        delete impl;
}

the count gets incremented in copy construction:

QDomNode::QDomNode(const QDomNode &n)
{
    impl = n.impl;
    if (impl)
        impl->ref.ref();
}

and in assignment as well:

QDomNode& QDomNode::operator=(const QDomNode &n)
{
    if (n.impl)
        n.impl->ref.ref();
    if (impl && !impl->ref.deref())
        delete impl;
    impl = n.impl;
    return *this;
}

thus method A is legal and safe, and bears no memory handling concerns.

I would also point out that using QMap::insert instead of the subscript operator is a bit more performant.

Doing:

QDomDocument doc;
doc.setContent( &file );
docs["alpha"] = doc;

or

docs["alpha"] = QDomDocument();
docs["alpha"].setContent( &file );

will both produce this:

  • a QDomDocument object is created (a temporary, in the second snippet)
  • another QDomDocument object is created (inside the map) calling docs["alpha"]
  • the latter is assigned to the first.

Using 

docs.insert("alpha", QDomDocument());
docs["alpha"].setContent( &file );

will only call the temporary constructor and the map item copy-constructor.

p-a-o-l-o
  • 9,807
  • 2
  • 22
  • 35
0

It's very clear that your QDomDocument is deleted when the scope ends:

{ // scope starts here 
    // create document on the stack
    QDomDocument doc;
    ...
    docs["alpha"] = doc;
} // scope ends here, stack is cleaned, doc is deleted

Creating QDomDocument on the heap solves the problem but might not be the best solution. For example, this should also work fine:

{
    ...
    docs["alpha"] = QDomDocument();
    docs["alpha"].setContent( &file );
    ...    
}
fonZ
  • 2,428
  • 4
  • 21
  • 40