Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
On Nov. 10, 2014, 9:41 p.m., David Faure wrote: lib/konq/src/konq_historyentry.h, line 57 https://git.reviewboard.kde.org/r/121080/diff/1/?file=327432#file327432line57 const ref, no? Andrius da Costa Ribas wrote: 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? David Faure wrote: Clearly this is not KDE specific. Any QListcustom 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 QListQStorageInfo 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? Andrius da Costa Ribas wrote: There is some historic reference here: http://marc.info/?l=kde-core-develm=113069150408826w=2 MSVC 2013 still has the same beahviour. 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. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70214 --- On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
On Nov. 10, 2014, 9:41 p.m., David Faure wrote: lib/konq/src/konq_historyentry.h, line 57 https://git.reviewboard.kde.org/r/121080/diff/1/?file=327432#file327432line57 const ref, no? Andrius da Costa Ribas wrote: 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? David Faure wrote: Clearly this is not KDE specific. Any QListcustom 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 QListQStorageInfo 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? There is some historic reference here: http://marc.info/?l=kde-core-develm=113069150408826w=2 MSVC 2013 still has the same beahviour. - Andrius da Costa --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70214 --- On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
On Nov. 10, 2014, 9:41 p.m., David Faure wrote: lib/konq/src/konq_historyentry.h, line 57 https://git.reviewboard.kde.org/r/121080/diff/1/?file=327432#file327432line57 const ref, no? Andrius da Costa Ribas wrote: 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? Clearly this is not KDE specific. Any QListcustom 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 QListQStorageInfo 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? - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70214 --- On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
On Nov. 10, 2014, 9:41 p.m., David Faure wrote: lib/konq/src/konq_historyentry.h, line 57 https://git.reviewboard.kde.org/r/121080/diff/1/?file=327432#file327432line57 const ref, no? 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? - Andrius da Costa --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70214 --- On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
On Nov. 9, 2014, 9:52 p.m., Alexander Richardson wrote: 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? KDEPimLibs also depends on this macro in several places. - Nicolás --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70152 --- On Nov. 8, 2014, 7:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 7:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70214 --- lib/konq/src/konq_historyentry.h https://git.reviewboard.kde.org/r/121080/#comment49091 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 QListKonqHistoryEntry (whose toSet method requires a qHash implementation) lib/konq/src/konq_historyentry.h https://git.reviewboard.kde.org/r/121080/#comment49092 const ref, no? - David Faure On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70149 --- Shouldn't it be actually implemented then? - Aleix Pol Gonzalez On Nov. 8, 2014, 10:26 p.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 10:26 p.m.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/#review70152 --- 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? - Alexander Richardson On Nov. 8, 2014, 11:26 nachm., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- (Updated Nov. 8, 2014, 11:26 nachm.) Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121080/ --- Review request for KDE Base Apps, KDE Frameworks and kdewin. Repository: kde-baseapps Description --- Since we're not using kdemacros.h here anymore, KDE_DUMMY_QHASH_FUNCTION is not available. Implement it instead. Diffs - lib/konq/src/konq_historyentry.h de34d6b Diff: https://git.reviewboard.kde.org/r/121080/diff/ Testing --- 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). Thanks, Andrius da Costa Ribas ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
