Replace KDE_DUMMY_QHASH_FUNCTION.

Review Request #121080 - Created Nov. 8, 2014 and updated

Information
Andrius da Costa Ribas
kde-baseapps
frameworks
121078
Reviewers
kde-baseapps, kdeframeworks, kdewin

Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead.

It builds (MSVC2013 - 64bit) after this patch (along other patches I'm sending to review today). Kdebase-apps is still not very functional, though (missing icons and weird UI).

Issues

  • 2
  • 0
  • 0
  • 2
Description From Last Updated
Just wondering: the place I found in Qt which does something similar (dummy qHash for MSVC), doesn't have any ifdef: ... David Faure David Faure
const ref, no? David Faure David Faure
Aleix Pol Gonzalez

Shouldn't it be actually implemented then?

Alex Richardson

When I originally got KF5 to compile on windows I seem to remember having to do this a few times manually as well.

Would it make sense to add the macro to KCoreAddons?

  1. KDEPimLibs also depends on this macro in several places.

David Faure

   
lib/konq/src/konq_historyentry.h (Diff revision 1)
 
 

Just wondering: the place I found in Qt which does something similar (dummy qHash for MSVC), doesn't have any ifdef: qitemselectionmodel.h. Any idea why we have one here?
It seems to me that this is useful for both the lib and the app, since they both see the QList<KonqHistoryEntry> (whose toSet method requires a qHash implementation)

lib/konq/src/konq_historyentry.h (Diff revision 1)
 
 

const ref, no?

  1. before I try to fix the pending issues: what are we going to decide?

    • Should we create KDE_DUMMY_QHASH_FUNCTION macro again? (which header)
    • Should it apply to MSVC-only or should it be ifdef-free?
  2. Clearly this is not KDE specific. Any QList<custom type> requires this on MSVC, right? Then I would strongly suggest a solution within Qt itself, if a central solution such as a macro is indeed needed. But thinking about it, a one-line dummy impl that returns 0 doesn't really seem worth a macro to me.
    I.e. why not do like qitemselectionmodel.h does, everywhere where this is needed?

    But I am still surprised that Qt only needs this in qitemselectionmodel.h
    Take this for instance:
    src/corelib/io/qstorageinfo.h: static QList<QStorageInfo> mountedVolumes();
    Why doesn't this require a qHash(QStorageInfo)?
    If I explicitly call toSet() on such a list, I do get a compile error (on Linux) due to the missing qHash implementation. http://www.davidfaure.fr/2015/storageview.diff
    So MSVC doesn't always instanciate the toSet() method, but only in some cases?
    Or are we looking at an old MSVC issue which doesn't exist anymore with more recent MSVC versions? i.e. did you try removing this block (in konq_historyentry.h) altogether to check it's still needed?

  3. There is some historic reference here: http://marc.info/?l=kde-core-devel&m=113069150408826&w=2
    
    MSVC 2013 still has the same beahviour.
  4. I know it's because MSVC instanciates all methods. This is why I don't understand why it works for e.g. QStorageInfo.

    Sounds to me like further research is needed, I don't like fixes where we don't really understand what's going on.

Loading...