Why does std::sort crash when working with a QList?

Question

I get a couple different crashes running the below code, sometimes a segmentation fault and sometimes ASSERT: "&other != this" in file C:/Qt/5.12.3/mingw73_64/include/QtCore/qstring.h, line 958.

The code to reproduce this is below.

#include <QList>
#include <QDebug>

class Tag
{
public:
    Tag(const QString& name)
    {
        this->name = new QString(name);
    }

    Tag(const Tag& tag)
    {
        this->name = new QString(tag.getName());
    }

    ~Tag()
    {
        if (name != nullptr) delete name;
    }

    QString getName() const
    {
        return *name;
    }

    static bool sortByName(const Tag& t1, const Tag& t2)
    {
        return t1.getName() < t2.getName();
    }

private:
    QString* name;
};

int main(int argc, char **argv)
{
    QList<Tag> tags;

    Tag a("a");
    Tag b("b");
    Tag c("c");
    Tag d("d");
    Tag e("e");

    tags << e << c << a << b << d;

    qDebug() << "before sorting";
    for (Tag tag : tags) {
        qDebug() << tag.getName();
    }

    std::sort(tags.begin(), tags.end(), Tag::sortByName);

    qDebug() << "after sorting";
    for (Tag tag : tags) {
        qDebug() << tag.getName();
    }

    return 0;
}

I have noticed that if the initial QList is filled with pointers to Tag objects and sortByName() is changed to accept pointers instead, it does not cause issues. I'm at a loss as to why.

What rafix07 is trying to say is that your `Tag` class has a rule of 3/5/0 violation. — David Schwartz, Aug 19 '19 at 04:02
Off-topic: it's legal to delete a null pointer, so you don't need to check in destructor... Any reason for not aggregating the string directly (`private: QString name;`)? That would make your life much easier... — Aconcagua, Aug 19 '19 at 04:06
Thank you all for the help! rafix07's suggestion solved my issue, and I will give the stack overflow thread linked by Nemo a read, thanks again. — Corvus, Aug 19 '19 at 04:06
@Aconcagua I may have been falsely taught that you should use pointers wherever possible as otherwise you may get a stack (or was it heap?) overflow. Is there no benefit to doing so? — Corvus, Aug 19 '19 at 04:09
Well, it depends... If you have large arrays of objects, or if you have really huge objects, stack overflow *can* occur. But QString itself is a rather small object, that won't hurt on the stack (data is maintained internally on the heap anyway). Additionally: The QString is embedded in the Tag class anyway, you still can decide having the `Tag` on stack or heap (together with the embedded string) just as you like... — Aconcagua, Aug 19 '19 at 04:16
@Aconcagua Thank you for the help, I'm glad the problem here was solved, but I do believe you are right and the best thing would be to make the QString not a pointer. — Corvus, Aug 19 '19 at 04:24
By the way: In embedded environments, you usually avoid dynamic allocation completely, as there are other dangers it can bring with it (memory fragmentation, for instance). — Aconcagua, Aug 19 '19 at 04:25
Yeah, `QString` shouldn't be a pointer like... ever. It uses copy-on-write sematics anyway and stores the text in heap, so using `new` on it is basically double overhead. — hyde, Aug 19 '19 at 04:59

selbie · Accepted Answer · 2019-08-19T04:31:30.827

4

The root of the problem is that you are using a raw pointer as a member variable (name) and trying to explicitly do a manual allocation/copy without the rule of 3 being satisfied. sort is invoking the copy assignment operator and getting the default implementation - which is to blindly copy the pointer value. Hence, double delete and crash...

The simplest fix is to just change name from a pointer to a concrete instance.

That is, this compiles and works fine with the main you already have provided.

class Tag
{
public:
    Tag(const QString& name) : _name(name)
    {
    }

    const QString& getName() const
    {
        return _name;
    }

    static bool sortByName(const Tag& t1, const Tag& t2)
    {
        return t1.getName() < t2.getName();
    }

private:
    QString _name;
};

If you actually need to keep name as a pointer, then add a copy assignment operator method to your Tag class:

class Tag
{
    …

    Tag& operator=(const Tag& t)
    {
        Qstring* oldName = this->name;
        this->name = new QString(t.getName());
        delete oldName;
        return *this;
    }
};

edited Aug 19 '19 at 04:31

answered Aug 19 '19 at 04:20

selbie

100,020
15
103
173

1

Uh, oh – what, if `this` already contained another string (`operator=`)? – Aconcagua Aug 19 '19 at 04:27
Would `this->name = new QString(t.getName());` result in a memory leak? – Corvus Aug 19 '19 at 04:28
1

Good callout. Fixed. And yes, I deleted the pointer after overwriting assignment in case of self-assignment. – selbie Aug 19 '19 at 04:32
1

@Corvus Important (that's done correctly here): *Always* create the new object first *before* you delete the old one; this way you're object remains in a valid state in case of an exception occurring on construction (e. g. `std::bad_alloc`). – Aconcagua Aug 19 '19 at 04:38

Why does std::sort crash when working with a QList?

1 Answers1