"Attempting to reference a deleted function" after adding QMutex to class

I am building an application with Qt5. My program builds and runs fine, but there is a collision between two threads accessing a data structure. I have a QList of CanMessage objects, and I want to protect some data inside of it using a QMutex. However, as soon as I add the QMutex to my class definition, I get errors:

QList.h: `error: C2280: 
'CanMessage::CanMessage(const CanMessage &)': attempting to reference a deleted function`.

Here is my canmessage.h file:

#ifndef CANMESSAGE_H
#define CANMESSAGE_H

#include <QObject>
#include <QMutex>
#include "cansignal.h"

class CanMessage
{
public:
    CanMessage();
    /* snip - public function prototypes */

private:
    /* snip - private prototypes and data */
    QMutex m_messageMutex;
};

#endif // CANMESSAGE_H

And cansignal.h:

#ifndef CANSIGNAL_H
#define CANSIGNAL_H

#include <QObject>
#include <QDebug>
#include <QByteArray>

class CanSignal
{
public:
    CanSignal();

    CanSignal(QString &signalName, quint8 &signalLength, quint8 &signalStartBit,
                   float &offset, float &factor, bool &isBigEndian, bool &isFloat, bool &isSigned)
    {
        this->m_name = signalName;
        this->m_length = signalLength;
        this->m_startBit = signalStartBit;
        this->m_offset = offset;
        this->m_factor = factor;
        this->m_isBigEndian = isBigEndian;
        this->m_isFloat = isFloat;
        this->m_isSigned = isSigned;
    }

    bool setName(QString &signalName);
    bool setBitLength(quint8 &length);
    bool setStartBit(quint8 &startBit);
    bool setOffset(float &offset);
    bool setFactor(float &factor);
    bool setEndianess(bool &isBigEndian);
    bool setIsFloat(bool &isFloat);
    bool setIsSigned(bool &isSigned);
    void setValid();
    void setInvalid();
    void setEngineeringData(float data);

    QString getName();
    quint8 getBitLength();
    quint8 getStartBit();
    float getOffset();
    float getFactor();
    float getData();
    bool isBigEndian();
    bool isFloat();
    bool isSigned();
    bool getSignalValidity();


private:
    QString m_name;
    quint8 m_length;
    quint8 m_startBit;
    float m_offset;
    float m_factor;
    float m_data_float = 0;
    bool m_isBigEndian;
    bool m_isFloat;
    bool m_isSigned;
    // Set After everything in signal is filled
    bool m_isSignalValid = false;
};

#endif // CANSIGNAL_H

Solution

CanMessage::CanMessage(const CanMessage &)

is the copy constructor, obviously being used to place an item into the list. That's not going to work since QMutex is not actually copyable.

How you solve it depends on a number of things. Perhaps the easiest method would be to modify CanMessage so that it has a dynamic mutex (created in the constructor, of course).

Then have a copy constructor for it that first locks the source object mutex then dynamically allocates a new mutex in the target object.

That way, you can guarantee the old object will be "clean" when copying (because you have its mutex) and there'll be no "trying to copy an uncopyable member" problem since the mutex itself is not copied. See footnote (a) for details.

The following code, which is a complete simple snippet showing the problem, compiles okay provided you leave the QMutex m_mutex; line commented out:

#include <QList>
#include <QMutex>

#include <iostream>

class Xyzzy {
private:
    int m_xyzzy;
    //QMutex m_mutex;
public:
    Xyzzy() : m_xyzzy(0) {};
    Xyzzy(int val) : m_xyzzy(val) {};
};

int main() {
    QList<Xyzzy> myList;
    Xyzzy plugh;
    myList.push_back(plugh);
    return 0;
}

Once you un-comment that line, the compiler rightly complains:

 error: use of deleted function 'Xyzzy::Xyzzy(const Xyzzy&)'

^(a) In terms of fixing the problem, you could do something like:

#include <QList>
#include <QMutex>

#include <iostream>

class Xyzzy {
private:
    int m_xyzzy;
    QMutex *m_mutex; // Now a pointer
public:
    Xyzzy() : m_xyzzy(0) {
        m_mutex = new QMutex(); // Need to create in constructor.
        std::cout << "constructor " << m_mutex << '\n';
    };
    ~Xyzzy() {
        std::cout << "destructor " << m_mutex << '\n';
        delete m_mutex; // Need to delete in destructor.
    }
    Xyzzy(const Xyzzy &old) {
        old.m_mutex->lock();
        m_mutex = new QMutex(); // Need to make new one here.
        std::cout << "copy constructor from " << old.m_mutex
                  << " to " << m_mutex << '\n';
        old.m_mutex->unlock();
    }
};

int main() {
    QList<Xyzzy> myList;
    Xyzzy plugh;
    myList.push_back(plugh);
    return 0;
}

That one works properly, as per the following test run:

constructor 0x21c9e50
copy constructor from 0x21c9e50 to 0x2fff2f0
destructor 0x21c9e50
destructor 0x2fff2f0

In real code, I'd probably opt for smart pointers rather than raw new/delete calls but this is only meant to illustrate the concept. In addition, you'd need to handle all other possibilities which create a new object from an existing one as per the rule of three/five/whatever-comes-next, currently (from memory) limited to the copy assignment member Xyzzy &operator=(const Xyzzy &old).