Review Request 121080: Replace KDE_DUMMY_QHASH_FUNCTION.

[David Faure]

> 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 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?
> 
> Andrius da Costa Ribas wrote:
>     There is some historic reference here: http://marc.info/?l=kde-core-devel&m=113069150408826&w=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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-windows/attachments/20150425/2d0ce024/attachment.html>