c'tor breaks my QList.append()

mzimmers

Hi all -

I'm trying to add a c'tor to a class from which I construct a QList. Here's the gist of it:

class EquipmentItem
{
public:
    QString m_name;
    QString m_description;
    QString m_serialNbr;
    ...
    // the line below breaks my list.append
    EquipmentItem(EquipmentItem &item) {} 
}

QList<EquipmentItem> list;

// line below uses: void append(rvalue_ref t)
list.append({"Heater", 
            "RS 485",
            "12345"
            });

Builds and runs correctly if I don't have a c'tor defined. When I do, I get an error:

error: no matching function for call to 'QList<EquipmentItem>::append(<brace-enclosed initializer list>)'

along with several failed candidate messages.

I've tried variations on the c'tor, but I end up with the same error. I would assume that QList.append() is invoking a copy c'tor (?) so I don't understand why creating another c'tor would affect it. Obviously, at least one of my assumptions here is flawed.

Can someone explain what's happening behind the scenes of that append() call, and how I can correctly form a c'tor in order to obviate the issue?

Thanks...

Chris Kawa

@mzimmers First of all copy constructor should take a const reference, so this:

EquipmentItem(const EquipmentItem &item) {}

but that's just a side note. It's not the issue here.

This

{"Heater",
 "RS 485",
 "12345"}

creates an std::initializer_list<const char*>. On the other hand append expects const QList<EquipmentItem>&.

QList<EquipmentItem> has a constructor that takes std::initializer_list<EquipmentItem>, so the only way this can work is if there exists an implicit conversion from const char* to EquipmentItem. But, since the only constructor you have for your type is a copy constructor from another item, there's no valid conversion, so compilation fails.

In short - if you want this append syntax you have to compile you need an item constructor like this:

EquipmentItem(const char* stuff) {  /* do something */ }

Chris Kawa

Just to add - what I said above would append 3 items. If you want just one item that gets created from that initializer list you have to provide a constructor that takes an initializer list, three strings or no constructor at all. If you don't have a constructor a compiler will initialize your class piece-wise from the initializer list. If you provide any constructor, be it default, custom, move or copy, then you prevent compiler from generating any automatic construction.

So another way to fix this is to remove all constructors or provide one of these:

EquipmentItem(const char* a, const char* b, const char* c) { ... }

EquipmentItem(const std::initializer_list<const char*>&) { ... }

mzimmers

@Chris-Kawa wow...lots to digest here.

QList<EquipmentItem> has a constructor that takes std::initializer_list<EquipmentItem>

Oh, so that's what that is. I thought somehow it was converting the content between the curly braces to an object and invoking the copy c'tor. So, I guess I got away with this because all my members could be considered <const char *>, and if I had an int member, it wouldn't have worked?

In any event, I'm not at all married to my current implementation - that's going to get thrown away anyway once my back-end is defined (so to speak). Do you have a recommendation for a better way to do this? My only requirement is that upon construction, I need to populate a QUuid member. This is why I was trying to add my own c'tor in the first place.

Thanks...

Chris Kawa

@mzimmers said:

if I had an int member, it wouldn't have worked?

You can have an int, or any other types there. It's so called list initialization. The point is that it's an automatically generated initialization. If you add any constructor, even an unrelated one, it disables any and all automatic construction mechanisms.

mzimmers

@Chris-Kawa OK, I think I get it. So, the QList.append() invokes an automatic construction mechanism (to use your term), which is only available if I don't try to help out by creating my own c'tors?

Very interesting.

So, how might you recommend doing this? Should I just go ahead and create a c'tor that accepts arguments for each member that I want to initialize externally? (EDIT: I realize I'll have to change my append() calls, probably (re)using a EquipmentItem variable that I initialize for each instance I want to create.)

Thanks...

Chris Kawa

@mzimmers said:

So, how might you recommend doing this?

If you don't need any custom behavior just don't add a constructor at all. It will "just work"™️. See the rule of zero and the rules of three/five for recommendations about providing initialization methods to your classes. Here's an example. In short - if you don't need custom behavior don't provide custom methods.

So, the QList.append() invokes an automatic construction mechanism

No, append doesn't do anything special. It just takes either a list or a single value. The value for QList<T> needs to be T, so if you provide anything else (like the {} initializer) it needs to be convertible to T to compile.

The list or its methods are not important here. It's exactly the same without any list, i.e. if you wrote

EquipmentItem item {"Heater", "RS 485", "12345"};

It will compile fine if you don't provide any constructors or if you provide one of the ones I mentioned. If you provide just a copy constructor this will not compile, because there's no copy here and you disabled automatic initialization by providing a constructor.

mzimmers

@Chris-Kawa OK, here's what I have so far (most extraneous stuff removed):

class EquipmentItem
{
    Q_GADGET
    Q_PROPERTY(QString name MEMBER m_name)

public:
    QUuid m_uuid;
    QString m_name;
    bool operator==(const EquipmentItem &rhs) const;
    EquipmentItem(QString name = "");
                 
EquipmentItem::EquipmentItem(QString name) :
    m_uuid(QUuid::createUuid()),
    m_name(name)
{}

EquipmentItem *item = new EquipmentItem("name");
m_equipmentList.append(*item);
delete item;

I've confirmed that it works, but I'd welcome feedback on whether this is a "best practices of C++" implementation, particularly where I create and delete the instance and then append it to the list.

Thanks...

Chris Kawa

@mzimmers said in c'tor breaks my QList.append():

I'd welcome feedback on whether this is a "best practices of C++" implementation, particularly where I create and delete the instance and then append it to the list

No, that's definitely bad. You're doing dynamic allocation just to copy the object and then immediately delete it. That's unnecessarily slow. Create the item on the stack if you need it also somewhere else, but if not then the best way to do it is to use QList::emplaceBack. This will create the item already on the list, so there would be no copies at all:

m_equipmentList.emplaceBack("name");

Some other notes about this constructor:

EquipmentItem(QString name = "")

Whenever you don't actually need a copy prefer a const reference. QString is implicitly shared, so copying it is not super expensive, but a const reference will be even faster still, since there's no copy made at all.
Another thing is the "". That's the most expensive way to make an empty QString. It calls a constructor that takes a string literal and copies it into its internal data storage. If you want an empty string just use a default constructor, that involves no copies, so the code should rather be:

EquipmentItem(const QString& name = QString())

mzimmers

@Chris-Kawa noted on the first point (I've included the other members):

    EquipmentItem(const QString &name = QString(),
                  const QString &description = QString(),
                  const QString &serialNbr = QString(),
                  const QUrl &iconFile = QUrl::fromEncoded(QByteArray()),
                  const Equipment::Category &category = Equipment::OTHER,
                  const QUrl &drawerFile = QUrl::fromEncoded(QByteArray()));

Regarding your second point, I assume this is what you meant:

EquipmentItem item (
                "name",
                "decrip",
                "12345",
                QUrl::fromEncoded("qrc:/icons/Pump.png"),
                Equipment::PUMP,
                QUrl("qrc:/Pump.qml")
                );

    m_equipmentList.emplaceBack(item);

Thanks for the feedback -- I sensed something was sub-optimal with my original implementation, but I couldn't put my finger on it.

Regarding the use of QList.append()...I followed an example from a video tutorial on this page. It's quite lengthy, so I don't expect you to watch it, but in the chance that you've already seen it, I'd be curious of what you think of the author's approach. And, I don't understand why he used append instead of emplace_back, either.

Chris Kawa

Regarding your second point, I assume this is what you meant:

No, that does a copy. If you use emplaceBack you pass all the constructor arguments directly to the function, so:

m_equipmentList.emplaceBack(
                "name",
                "decrip",
                "12345",
                QUrl::fromEncoded("qrc:/icons/Pump.png"),
                Equipment::PUMP,
                QUrl("qrc:/Pump.qml")
                );

the way it works is that append creates a new instance on the list and initializes it using a copy constructor. emplaceBack doesn't do a copy. It creates the item directly on the list using a normal constructor and passes all these arguments to it using so-called perfect forwarding.

As for the constructor, like with a QString, if you want an empty url just use default constructor, so const QUrl &iconFile = QUrl(). You don't need a roundtrip through creating an empty array and trying to decode it with fromEncoded.

For basic types, like enums, you don't need a reference. Copy costs the same in that case, so you can just do Equipment::Category category = Equipment::OTHER

mzimmers

@Chris-Kawa thanks for all the help on this. Feel free to add anything, but I'm going to consider the topic solved.