0

I'm looking for best practices with Qt smart pointers. Let's say I have something like this.

void aFunction() {
    QSharedPointer<QVector<QSharedPointer<Object> > > pointer = QSharedPointer<QVector<QSharedPointer<Object> > >(new QVector<QSharedPointer<Object> >);

    doSomethingWithPointer(pointer);
    changeOwnership(pointer);
}

Then in doSomethingWithPointer function

void doSomethingWithPointer(QSharedPointer<QVector<QSharedPointer<Object> > > pointer) {
    pointer->append(QSharedPointer<Object>(new Object);
}

Now I after doSomethingWithPointer, I pass the pointer to changeOwnership, which should take ownership of it. What I'm wondering about is doSomethingWithPointer. Is it okay to pass shared pointers in a function like that? Or should I maybe use a QWeakPointer since doSomethingWithPointer doesn't actually take ownership of pointer? When exactly should I use QWeakPointer?

joefuldoe
  • 35
  • 1
  • 5
  • 2
    The best practice with QTSmartPointers is to never use QTSmartPointers :) – SergeyA Jun 14 '16 at 13:47
  • 1
    [This](http://stackoverflow.com/questions/12030650/when-is-stdweak-ptr-useful) and [this](http://stackoverflow.com/questions/8385457/should-i-pass-a-shared-ptr-by-reference) pretty much sums it up. They are about the standard library, but what's the difference? – LogicStuff Jun 14 '16 at 13:56

1 Answers1

6

It doesn't make much sense to pass a weak pointer, since the only way to safely use a weak pointer is to cast it back to a shared pointer. Qt's api is somewhat broken in this respect as it allows you to use QWeakPointer::data() to cheat - but that's silly, you're never meant to use it that way. std::weak_ptr got it right and you cannot use it other than casting it to a std::shared_ptr.

But none of this is necessary anyway.

Qt's containers are implicitly shared value classes. Passing them around by pointers is superfluous. A QVector internally is a shared pointer to its data. When you want to cede its ownership, you can leverage move semantics to depessimize the implicit sharing that would otherwise occur.

The answer in your particular case is: don't use an external shared pointer at all.

struct Object {};

void appendObject(QVector<QSharedPointer<Object>> & data) {
  data.append(QSharedPointer<Object>(new Object));
}

void use(QVector<QSharedPointer<Object>> && data) {
}

int main() {
    QVector<QSharedPointer<Object>> data;
    appendObject(data);
    use(std::move(data));
}

Sidebar: In C++11 you'd expect the uniform initialization to work - but it doesn't since QVector and/or QSharedPointer have an incompatible API. So this would be nice, but no dice:

data.append({new Object});

It'd be acceptable to write this in Qt 4 style and depend on implicit sharing of Qt containers, but this style is a premature pessimization in Qt 5/C++11:

void use(QVector<QSharedPointer<Object>> & data) {
}

int main() {
    QVector<QSharedPointer<Object>> data;
    appendObject(data);
    use(data);
}

It is also worth investigating whether Object could work as a value class that could be stored directly in a container by value.

Even if that's not the case, if you don't really need shared ownership of Object, but merely a dynamically allocated container, you could use QList instead:

struct Object {};

void appendObject(QList<Object> & data) {
  data << Object();
}

void use(QList<Object> && data) {
}

int main() {
    QList<Object> data;
    appendObject(data);
    use(std::move(data));
}

If the object is not copyable, you should use std::list and leverage move semantics to change ownership:

void appendObject(std::list<QObject> & data) {
   data.emplace_back();
}

void use(std::list<QObject> && data) {
}

int main() {
   std::list<QObject> data;
   appendObject(data);
   use(std::move(data));
}

Finally, if the class you're managing is derived from QObject, you could leverage QObject being a QObject container:

void appendObject(QObject * container) {
  new QObject(container); // add a child
}

void use(QScopedPointer<QObject> && container) {
  for (auto object : container->children())
    qDebug() << object;
}

int main() {
  QScopedPointer<QObject> container;
  appendObject(container.data());
  use(std::move(container));
}

Since QObject internally uses PIMPL, we can too, and thus get rid of the silly external pointer. After all, a QObject is a QScopedPointer to its pimpl, just like most other Qt classes are!

#include <QtCore>
#include <private/qobject_p.h>

class Container : public QObject {
public:
   Container(QObject * parent = 0) : QObject(parent) {}
   Container(Container && other) :
      QObject(*new QObjectPrivate, other.parent()) {
      d_ptr.swap(other.d_ptr);
   }
};

void appendObject(Container & container) {
  new QObject(&container); // add a child
}

void use(Container && container) {
  for (auto object : container.children())
    qDebug() << object;
}

int main() {
  Container container;
  appendObject(container);
  appendObject(container);
  use(std::move(container));
}

And finally, if you expect the containers to be "small", i.e. the number of elements * element size is less than 1-4kbytes, then do not use std::list nor QList, but std::vector and QVector, respectively. Small lists perform extremely poorly, generally speaking, on any modern architecture with multi-level memory caching. They may be OK for a simple microcontroller without a memory cache, so we're talking Arduino performance levels here, but even then has to be measured first.

Kuba hasn't forgotten Monica
  • 88,505
  • 13
  • 129
  • 275