hash clean up

[daschuer]

Instead of using the std namespace we can use qHash() for all elements.

[Swiftb0y]

I disagree with this change. The original implementation is completely fine https://en.cppreference.com/w/cpp/language/extending_std#Adding_template_specializations

std::hash works for any hashmap, qHash locks us into Qt containers.

[daschuer]

Thank you for sharing the link:

Opening namespace std can accidentally introduce undefined behavior ...

I read it as opening a namespace as we original did is not recommend. So thats the issue we should solve.

I have decided to not use a mix of std and Qt functions here to keep the code consistent with itself and compared to other places. That way readers just need to look up the Qt docs and we have to deal with only one header.

Currently there is no need for std compatibity and if we decide one day to use a std container in place of QHash (which is the same anyway in Qt6) we should swap it everywhere IMHO.

What do you think?

[Swiftb0y]

I read it as opening a namespace as we original did is not recommend. So thats the issue we should solve.

I agree. though from what I can tell is that the warning is only there because if you're opening namespace std, its easy to add things to it that don't fit the exceptions, thus introducing UB. We should stick to std::hash and instead just write

template <>
std::hash<PixmapKey>

directly as recommended by cppreference.

I have decided to not use a mix of std and Qt functions here to keep the code consistent with itself and compared to other places. That way readers just need to look up the Qt docs and we have to deal with only one header.

I don't really see how that has a significant benefit.

Let me lay out my point on why we should prefer std solutions of Qt's in the majority of cases:
1.. std solutions are usually better designed, tested and maintained than Qt counterparts due to the larger userbase.
2. Compatibility with other 3rd party libraries is much better since the also target the standard library instead of Qt. The std is the greatest common divisor so to speak.
3. Project contributors should know C++ and as such the standard library. They're probably much more familiar with the tools available in std. Having to get familiar with the Qt tools (which are the std tools under the hood anyways) is an extra burden on them. The other way around (Qt developers not knowing std C++) does (or rather should) not happen. If they know neither, its better for them to learn modern C++ first (instead of Qt which works around the flaws of outdated C++ restrictions and they're API still suffers from that).

Exceptions:

1. The code in question deeply interacts with Qt APIs that do not accept std types (cases of this are decreasing with every new Qt version fortunately)
2. The Qt solution is significantly easier to use than the std solution (can't think of any concrete examples right now).

Does that make sense?

Currently there is no need for std compatibity and if we decide one day to use a std container in place of QHash (which is the same anyway in Qt6) we should swap it everywhere IMHO.

If we're okay with doing mass migrations like that sure... but I also don't see anything wrong with gradual adoption if possible. Especially in this particular case where Qt can work with both std::hash and qHash.

[daschuer]

I have now reverted to the std::hash version and fixed it. It turns out that we need a special version with a seed parameter for full Qt compatibility: https://github.com/qt/qtbase/blob/0b0b30f7cf900ea0463f4073f0e82d014920fd1d/src/corelib/tools/qhash.h#L60

[Swiftb0y]

LGTM. waiting for CI, thank you.