Review Request: Update PlaydarCollection and related components to use QWeakPointer

Ian Monroe ian.monroe at gmail.com
Tue Nov 9 15:28:42 CET 2010


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100141/#review329
-----------------------------------------------------------


Mostly gcc is the best reviewer for this sort of stuff. So the "it compiles? ... ship it!" philosophy works well. anyways after you incorporate the suggestions of mine that you want, just go ahead and merge.


src/core-impl/collections/playdarcollection/PlaydarCollection.cpp
<http://git.reviewboard.kde.org/r/100141/#comment276>

    for a pointer that immediately goes out of scope, there really isn't much point in using qweakpointer (or qpointer)



src/core-impl/collections/playdarcollection/PlaydarMeta.cpp
<http://git.reviewboard.kde.org/r/100141/#comment277>

    why not just "return (m_collection.data())"



src/core-impl/collections/playdarcollection/PlaydarMeta.cpp
<http://git.reviewboard.kde.org/r/100141/#comment278>

    same as above


- Ian


On 2010-11-08 19:58:42, Andy Coder wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100141/
> -----------------------------------------------------------
> 
> (Updated 2010-11-08 19:58:42)
> 
> 
> Review request for Amarok and Mark Kretschmann.
> 
> 
> Summary
> -------
> 
> Drops QPointer in favor of QWeakPointer in src/core-impl/collections/playdarcollection, as was done for other code in [0] and recommended in [1].
> 
> Nothing works differently, but since it is, in number of changes at least, non-trivial, (and I've been fairly absent for a while), I figured I ought to put it up for review.  In particular, anyone more familiar with QWeakPointer checking to see if I did something stupid that might cause hard to find problems down the road would be appreciated.
> 
> [0] - http://git.reviewboard.kde.org/r/100011/
> [1] - http://git.reviewboard.kde.org/r/100001/
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.h 31aba11 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.cpp 845d3fd 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.h 2599b7c 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.cpp 755191a 
>   src/core-impl/collections/playdarcollection/PlaydarQueryMaker.h 277a582 
>   src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp 590c0d8 
>   src/core-impl/collections/playdarcollection/support/Controller.h 1db079a 
>   src/core-impl/collections/playdarcollection/support/Query.h 01971f7 
>   src/core-impl/collections/playdarcollection/support/Query.cpp d586047 
> 
> Diff: http://git.reviewboard.kde.org/r/100141/diff
> 
> 
> Testing
> -------
> 
> Compiled, ran, played around. Worked fine for me.
> 
> 
> Thanks,
> 
> Andy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101109/a1b86ebd/attachment.htm 


More information about the Amarok-devel mailing list