QHash with QObject based value type produces unexpected results

Pl45m4

I'm using a QHash<QWidget*, MyData *>, where MyData is a QObject derived class.
The class is more like an extended struct with signals & slots (and needs to be a QObject type to be a value in a QHash).

Each MyData obj is hashed based on its m_id integer member.
Therefore I've implemented all the operators and hashing functions to make this possible.

However, if I compare my results with a plain QHash<QWidget*, int> hash, they don't match.

The integer hash works as expected, while the MyData * hash produces random/undefined results.
(Example down below)

Can't figure out what I'm doing wrong here.

See Test Case #2 vs. Test Case #4
I expect these two to output the same, which, for some reason, isn't the case.
(I'm aware that QHash entries are not sorted)

main.cpp

#include <QApplication>
#include <QHash>
#include <QWidget>
#include <QDebug>


class MyData : public QObject
{
    Q_OBJECT

public:

    MyData(int id, QObject *parent = nullptr): QObject(parent), m_id(id){}

    int id() const { return m_id; }


private:

    int m_id;

};

// operators for mapping and comparison (std::max_element, ... )
inline bool operator<(const MyData &data1, const MyData &data2){ return data1.id() < data2.id(); }
inline bool operator==(const MyData &data1, const MyData &data2){ return data1.id() == data2.id(); }
// hashing functions
inline size_t qHash(const MyData &key, size_t seed){ return qHash(key.id(), seed); }
inline size_t qHash(MyData &key, size_t seed){ return qHash(key.id(), seed); }


QHash<QWidget *, int> intMapping;
QHash<QWidget *, MyData *> dataMapping;


void testFunctionData(QWidget *widget, int id, QObject *parent)
{
    MyData *data = nullptr;
    if (id == -1) {
        QHash<QWidget *, MyData *>::iterator it
            = std::max_element(dataMapping.begin(), dataMapping.end());

        if (it == dataMapping.end()) { // List empty

            data = new MyData(0, parent);
            qDebug() << "List empty -> New Data Obj starting with ID:" << data->id();
            dataMapping[widget] = data; // create new hashmap entry
        }
        else { // else, get next number

            int currMax = it.value()->id();
            qDebug() << "max_element in list has ID:" << currMax;

            int nextID = currMax + 1;
            qDebug() << "New Data Obj with next free ID created:" << nextID;
            data = new MyData(nextID, parent); // create obj with ID
            dataMapping[widget] = data; // create new hashmap entry
        }

    } else {

        data = new MyData(id, parent);
        qDebug() << "New Data Obj with user defined ID created:" << data->id();
        dataMapping[widget] = data; // create new hashmap entry
    }

}


void testFunctionInteger(QWidget *widget, int id)
{
    if (id == -1) {
        // max element = highest ID
        // => std::max_element compares QHash values (=> integers)
        const QHash<QWidget *, int>::const_iterator it
            = std::max_element(intMapping.cbegin(), intMapping.cend());

        // if iter reaches end, list empty
        // -> to-be-added widget gets ID 0
        if (it == intMapping.cend())
            intMapping[widget] = 0;
        else // else, use next higher number
            intMapping[widget] = *it + 1;

    }
    else // assign ID passed with function to widget directly
        intMapping[widget] = id;

}


void printIntHashmap()
{
    QHashIterator<QWidget *, int> i(intMapping);
    while (i.hasNext()) {
        i.next();
        qDebug() << i.key() << ": int ID:" << i.value();
    }
}

void printDataHashmap()
{
    QHashIterator<QWidget *, MyData *> i(dataMapping);
    while (i.hasNext()) {
        i.next();
        qDebug() << i.key() << ": MyData ID:" << i.value()->id();
    }
}


int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    QObject obj;
    QList<QWidget*> widgetList;

    for (int i = 0; i < 5; i++) {
        QWidget *w = new QWidget;
        widgetList.push_back(w);
    }

    qDebug() << "##########################################################";
    qDebug() << "##### TEST CASE 1";
    qDebug() << "##########################################################";
// ##########################################################
// # TEST CASE 1:

    for (auto w : widgetList) {
        testFunctionInteger(w, 1); // force ID 1
    }
    printIntHashmap();
    intMapping.clear();

// ##########################################################
    qDebug() << "##########################################################";
    qDebug() << "##### TEST CASE 2";
    qDebug() << "##########################################################";
// ##########################################################
// # TEST CASE 2:

    for (auto w : widgetList) {
        testFunctionInteger(w, -1); // auto-assign next free ID
    }
    printIntHashmap();

// ##########################################################
    qDebug() << "##########################################################";
    qDebug() << "##### TEST CASE 3";
    qDebug() << "##########################################################";
// ##########################################################
// # TEST CASE 3:

    for (auto w : widgetList) {
        testFunctionData(w, 1, &obj);  // force ID 1
    }
    printDataHashmap();
    dataMapping.clear();

// ##########################################################
    qDebug() << "##########################################################";
    qDebug() << "##### TEST CASE 4";
    qDebug() << "##########################################################";
// ##########################################################
// # TEST CASE 4:

    for (auto w : widgetList) {
        testFunctionData(w, -1, &obj);  // auto-assign next free ID
    }
    printDataHashmap();

// ##########################################################
    qDebug() << "##########################################################";

    qDeleteAll(widgetList);
    return a.exec();
}
#include "main.moc"

This is my output:

##########################################################

TEST CASE 1 (correct)

##########################################################
QWidget(0x28299588a00) : int ID: 1
QWidget(0x2829958e710) : int ID: 1
QWidget(0x282995889a0) : int ID: 1
QWidget(0x2829958e140) : int ID: 1
QWidget(0x2829958e380) : int ID: 1
##########################################################

TEST CASE 2 (correct)

##########################################################
QWidget(0x28299588a00) : int ID: 1
QWidget(0x2829958e710) : int ID: 3
QWidget(0x282995889a0) : int ID: 0
QWidget(0x2829958e140) : int ID: 2
QWidget(0x2829958e380) : int ID: 4
##########################################################

TEST CASE 3 (correct)

##########################################################
New Data Obj with user defined ID created: 1
New Data Obj with user defined ID created: 1
New Data Obj with user defined ID created: 1
New Data Obj with user defined ID created: 1
New Data Obj with user defined ID created: 1
QWidget(0x28299588a00) : MyData ID: 1
QWidget(0x2829958e710) : MyData ID: 1
QWidget(0x282995889a0) : MyData ID: 1
QWidget(0x2829958e140) : MyData ID: 1
QWidget(0x2829958e380) : MyData ID: 1
##########################################################

TEST CASE 4 (not correct, should output like Case #2)

##########################################################
List empty -> New Data Obj starting with ID: 0
max_element in list has ID: 0
New Data Obj with next free ID created: 1
max_element in list has ID: 0
New Data Obj with next free ID created: 1
max_element in list has ID: 0
New Data Obj with next free ID created: 1
max_element in list has ID: 1
New Data Obj with next free ID created: 2
QWidget(0x28299588a00) : MyData ID: 1
QWidget(0x2829958e710) : MyData ID: 1
QWidget(0x282995889a0) : MyData ID: 0
QWidget(0x2829958e140) : MyData ID: 1
QWidget(0x2829958e380) : MyData ID: 2
##########################################################

In case you are wondering:
I want to store MyData in my hash "table", where I use the id integer member for all hashing and comparison operations, if that makes sense.
So it should behave like a QHash<QWidget*, int> hashmap.

Is my understanding how MyData is hashed wrong?
Am I doing something terribly wrong?
Am I tripping? :D
Spent almost the whole weekend trying to figure out how it works and why my code does not, apparently :D

Btw: the values of case #4 are completely random in each run but never match the expected results as in case #2.

The code should be compilable and work right away.
(It's a minimal example, the original code is more complex)

Windows 10 & Qt 6.7, minGW, C++17

Edit:

First I thought std::max_element might be the issue here, but as far as I understand:

Elements are compared using operator< (until C++20) std::less{} (since C++20).

( https://en.cppreference.com/w/cpp/algorithm/max_element )

It should work with MyData as I implemented the operator< for this specific reason... to compare the id integer members of MyData, which then should not make any difference to the QHash using the integers directly as its values.

Still confused :/

Any help appreciated :)

Pl45m4

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

Will try to add a new set of operators and check the output again...

Implementing

inline bool operator<(MyData *data1, MyData *data2){ return data1->id() < data2->id(); }

results in:

main.cpp:26:13: Overloaded 'operator<' must have at least one parameter of class or enumeration type

So it's not supported by any operator and C++ standard.

And because you also cannot store QObject types by value in a container (no copy/assignment c'tor), it's never going to work :(

I guess I have to think about my design again esp. how I'm going to map the information together and how to access them :/

Maybe I get rid of the QObject inheritance and store plain data "struct", which then would be mappable again...

Thanks to anyone who contributed :)

Edit:

As the discussion was progressed further:
This turns out to be one possible solution:

@VRonin said in QHash with QObject based value type produces unexpected results:

Bingo! that's your problem but you can just use the 3rd argument of std::max_element, no need for crazy stuff: QHash<QWidget *, MyData *>::iterator it = std::max_element(dataMapping.begin(), dataMapping.end(), [](MyData *a, MyData *b) -> bool {return a->id() < b->id();});

However, will see if I re-design this whole part of my app or just use it like this... :)

ChrisW67

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

Each MyData obj is hashed based on its m_id integer member.

Yes, but you are not placing a MyData object into the QHash: you are placing a pointer-to-MyData in the hash. Neither of your qHash() functions matches that signature. Inserting your int matches
qHash(int key, size_t seed = 0) and your pointer matches
qHash(const T *key, size_t seed = 0). These provide different results.

Pl45m4

@ChrisW67

D'oh!

Was so focused on that it part with std::max_element that I didn't even noticed.

Now I've added two more (const and not const) hashing functions which accept a pointer to MyData.

// hashing functions
inline size_t qHash(const MyData &key, size_t seed){ return qHash(key.id(), seed); }
inline size_t qHash(MyData &key, size_t seed){ return qHash(key.id(), seed); }
inline size_t qHash(const MyData *key, size_t seed){ return qHash(key->id(), seed); }
inline size_t qHash(MyData *key, size_t seed){ return qHash(key->id(), seed); }

BUT... unfortunately this doesn't seem to change anything... I'm still getting weird results in Case #4 :(
Commented the "wrong" hashing functions so that they have no impact... no changes.

Are you able to reproduce it?

Does, with the pointer hashing function, case #2 match case #4 now?

I still get something like:

##########################################################

TEST CASE 4

##########################################################
List empty -> New Data Obj starting with ID: 0
max_element in list has ID: 0
New Data Obj with next free ID created: 1
max_element in list has ID: 1
New Data Obj with next free ID created: 2
max_element in list has ID: 1
New Data Obj with next free ID created: 2
max_element in list has ID: 2
New Data Obj with next free ID created: 3
QWidget(0x20ebb5b8700) : MyData ID: 1
QWidget(0x20ebb5be1c0) : MyData ID: 3
QWidget(0x20ebb5be730) : MyData ID: 2
QWidget(0x20ebb5b87f0) : MyData ID: 0
QWidget(0x20ebb5be6a0) : MyData ID: 2
##########################################################

VRonin

What gets hashed is the key, not the value. The hash you are reading are of QWidget*

Pl45m4

@VRonin

That explains why changing the qHash function doesn't solve it. :D
So how to deal with it?

I mean, if MyData entries are not hashed, why I'm not able to compare their id?

VRonin

It depends on what you are trying to do. From what you describe it looks like what you actually want is a QHash<int,MyData*> where the key is my_daya.id().

What is not clear from above is how you are actually using the current key i.e. the QWidget*

Pl45m4

@VRonin said in QHash with QObject based value type produces unexpected results:

QHash<int,MyData*>

Why would you need this, when int ID is a member in MyData?

Actually I took the idea from the implementation of QButtonGroup.
QAbstractButton - widgets are mapped to their groupID the same way I'm trying to do... with that one difference:
As used in my "working" cases, they are mapped to the plain int ID's and not to a struct/class which contains the ID.
Like QHash<QAbstractButton *, int>, where int is the groupID.

Where I thought of:
QHash<QWidget *, MyData.id() > (makes no sense written like this, but you see what I'm trying)

I (think I) need to store a pointer to the complete class MyData, because I want to let MyData emit signals depending on the look-up results and not only store the ID.

Otherwise I would need 2-3 more maps/hashes?!
Map Widget to ID, map ID to another class (MyData), then do something and map it to my initial Widget...
That doesn't sound right/like a good design, though.

VRonin

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

because I want to let MyData emit signals depending on the look-up results

If your lookup is based on QWidget * then you don't need to hash MyData because that's not what you are looking up.

If you want a hash that can search for either QWidget* or MyData* then you need a bidirectional container like KBiHash (https://invent.kde.org/frameworks/kitemmodels/-/blob/master/src/core/kbihash_p.h#L543)

Pl45m4

@VRonin said in QHash with QObject based value type produces unexpected results:

If your lookup is based on QWidget * then you don't need to hash MyData because that's not what you are looking up

So why are my results for <QWidget *, MyData*> and <QWidget *, int> different then?
In both cases I compare the ID (int).
First one via MyData::id(), second one by reading the int value from the hash directly.

From my understanding...
If in both cases the only the QWidget is hashed, then there should be no problem with the hash itself... and my logic, what I'm doing afterwards (see testFunction[Data|Int]) is completely identical.

Am I still doing something wrong there?

Has anyone tried my sample code?

Edit:

While writing this, I think I know...
std::max_element in both cases iterates the hash and seems to compare the value of all keys, returning the largest/highest value...
That means, even if I use it.value().id() to get the actual ID from MyData.... The object I'm calling this function on, is not the one with the highest ID. If I'm not mistaken, std::max_element compares the pointers to MyData and returns the highest value (~~I suspect the highest address to win~~)... since I don't have a specific operator < implemented, which tells that my intention is to compare MyData * by using MyData::id().

Will try to add a new set of operators and check the output again...

Edit_2:

Moving to C++20 so that std::max_element uses std::less might be also worth a try. Still need to implement comparison operator. Otherwise they are ordered by:

https://en.cppreference.com/w/cpp/language/operator_comparison#Pointer_total_order

VRonin

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

std::max_element in both cases iterates the hash and seems to compare the value of all keys

Nope, it uses QHash<QWidget *, MyData *>::iterator::operator*() so it compares all values not all keys

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

std::max_element compares the pointers to MyData and returns the highest value (I suspect the highest address to win)... since I don't have a specific operator < implemented, which tells that my intention is to compare MyData * by using MyData::id().

Bingo! that's your problem but you can just use the 3rd argument of std::max_element, no need for crazy stuff: QHash<QWidget *, MyData *>::iterator it = std::max_element(dataMapping.begin(), dataMapping.end(), [](MyData *a, MyData *b) -> bool {return a->id() < b->id();});

Pl45m4

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

Will try to add a new set of operators and check the output again...

Implementing

inline bool operator<(MyData *data1, MyData *data2){ return data1->id() < data2->id(); }

results in:

main.cpp:26:13: Overloaded 'operator<' must have at least one parameter of class or enumeration type

So it's not supported by any operator and C++ standard.

And because you also cannot store QObject types by value in a container (no copy/assignment c'tor), it's never going to work :(

I guess I have to think about my design again esp. how I'm going to map the information together and how to access them :/

Maybe I get rid of the QObject inheritance and store plain data "struct", which then would be mappable again...

Thanks to anyone who contributed :)

Edit:

As the discussion was progressed further:
This turns out to be one possible solution:

@VRonin said in QHash with QObject based value type produces unexpected results:

Bingo! that's your problem but you can just use the 3rd argument of std::max_element, no need for crazy stuff: QHash<QWidget *, MyData *>::iterator it = std::max_element(dataMapping.begin(), dataMapping.end(), [](MyData *a, MyData *b) -> bool {return a->id() < b->id();});

However, will see if I re-design this whole part of my app or just use it like this... :)

VRonin

@Pl45m4 said in QHash with QObject based value type produces unexpected results:

So it's not supported by any operator and C++ standard.

Yes, overloading operators for built in types (pointers in this case) is explicitly forbidden by the standard. Hence why I use a lambda above. If you are pre-C++11 then you can use functor:

class MyDataCompare{
public:
bool operator()(MyData *data1, MyData *data2) const { return data1->id() < data2->id(); }
}

Pl45m4

@VRonin said in QHash with QObject based value type produces unexpected results:

If you are pre-C++11 then you can use functor:

Pre-C++11?!
So C++0X or C++98?!
I don't know if I want to use a C++ Standard this old.
Doesn't Qt6 need at least C++11 or something to work?

I'm still wondering what I could do, now that I know that my initial idea isn't working.

VRonin

@Pl45m4 maybe I’m missing something but I did suggest a solution above (passing a lambda as extra argument to max_element) is that not feasible for your code?

Pl45m4

@VRonin

I misread your reply.
Was looking at the functor code and thought, huh, why you need C++11 or below.
Missed that you referred with "lambda" to the code sample 1-2 posts earlier.

Yes, the lambda, in which I tell std::max_element how to compare, works.
I still take this as a chance to re-think what I'm doing and if there is a better way.

Thank you :)