Review Request 111421: kmix: Use QSharedPointer instead of std::shared_ptr.

[Christian Esken]

> On Aug. 20, 2013, 10:38 a.m., Raphael Kubo da Costa wrote:
> > Ping.

Pong :-)
Exchanging one shared pointer with another is not a decision to take lightly - the bugs radiating around objects deletions are extremely nasty and time consuming.  As I reached stability I would not really want too risk it again. I also like use_count() from shared_ptr. Fixing #include statements is way easier, faster and more secure (does not compile vs breaks during runtime) than fixing a broken rutime.
If I remember correctly I had problems with using QSharedPointer. I have a loop in my Object graph (Mixer <-> DBUSMixerWrapper or DBUSControlWrapper) and Qt would start deleting the Object that I wanted to save. Issues very especially hard when Qt began running "slots" for async signals at inconvenient times (during emit() IIRC). Might have had a different root cause (and design issue) though.

Summarizing: It would be possible and has advantages for compiling, but I do not think it is worth the risk during runtime.


- Christian


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


On July 6, 2013, 5:35 p.m., Raphael Kubo da Costa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111421/
> -----------------------------------------------------------
> 
> (Updated July 6, 2013, 5:35 p.m.)
> 
> 
> Review request for KDE Multimedia and Christian Esken.
> 
> 
> Description
> -------
> 
> std::shared_ptr is a C++11 feature, whose location (and existence) in STL
> implementations of previous C++ standards varied -- including <tr1/memory>
> unconditionally breaks the build if libc++ is used instead of libstdc++, for
> example.
> 
> While it could be possible to just check which headers are available and
> where std::shared_ptr is defined and include only the proper headers, I
> don't see any reason not to just use QSharedPointer instead.
> 
> 
> Diffs
> -----
> 
>   apps/kmix.cpp 8fdcf730789eb2b227f199c6cbb77db8545d918d 
>   apps/kmixd.cpp 442abaf89b65f96382bf954e6cb592d61ff12b12 
>   backends/mixer_alsa.h c7f49f2f623b4741317a9aea1e85ceb219ed6b62 
>   backends/mixer_alsa9.cpp 635a20162b9eda45ca2c9ef0c2d067b5469ab403 
>   backends/mixer_backend.h 642fe70df314009b3d748666bb03abd8d61f1ab7 
>   backends/mixer_backend.cpp f4758d673cbbc7c9d65dd4a134d82df183b7dbd6 
>   backends/mixer_mpris2.h 18e2b1835bc9fec0f78c6cdf365a27f3f352d07e 
>   backends/mixer_mpris2.cpp 18641258f63b961e0a3f0cdf01a9e21d0f0c42be 
>   backends/mixer_oss.h 4f2646a860d86759cef64d3dd13d8c639c028d4c 
>   backends/mixer_oss.cpp 28e972d4d33b040e5597acd4d6d8c51c9c90a82c 
>   backends/mixer_oss4.h ae509318b130db0a382b82fcb433f3ccacdc6d10 
>   backends/mixer_oss4.cpp 058d999d366e13850953666b0b5a4c24f0d109a9 
>   backends/mixer_pulse.h ef2050c561cfc8669939cc844750034dd87bed4e 
>   backends/mixer_pulse.cpp d043ec4a3067caacdfda1267a558fc4d44d3e7f8 
>   backends/mixer_sun.h 4412e442c5b4f421ecd4518435ab7fc67e31c085 
>   backends/mixer_sun.cpp 62edc3fb8fb8832ccf207953c0b053b0107a7421 
>   core/ControlPool.h 4cb2222f46e9ed855bfbe028fe1c124f683d8f8a 
>   core/ControlPool.cpp f2dd17b4f7e4a55c738bb1091671e07c7812c48c 
>   core/MasterControl.h dff9e95d719508a8966d1d9c994948851d5bd2c8 
>   core/mixdevice.h 177c3b264683bb967b87f08a1b83855c3295ea7f 
>   core/mixdevice.cpp 06b883ea70030d3e3abc2727674b18cff7a181e1 
>   core/mixdevicecomposite.h 0180e2450bc2d9feb2b9d5e237f6ecd948f38806 
>   core/mixdevicecomposite.cpp dec3a8f69c0c50384615c6765adc344b987c9d44 
>   core/mixer.h 97e2775e3c15f49bdaea3dd4e6748ef284df8950 
>   core/mixer.cpp f3e14ec2137a7a94ecc4760be2140a2e937161f4 
>   core/mixertoolbox.cpp 481e0f3255d3a2dc8142c5f8ba95f411e41802f4 
>   core/mixset.h acea5991bf20d113976c6758b40559f4b27b941a 
>   core/mixset.cpp 67cd2c58b5e6f00d8821c1273fa47f024092a87a 
>   dbus/dbuscontrolwrapper.h 5355f974d16ebff8218c7582d949d688050f7d97 
>   dbus/dbuscontrolwrapper.cpp 4e141c1bdb72bff330fb12952eeeddb61169484e 
>   dbus/dbusmixerwrapper.cpp d135d4209f1741ce5d678d0df11461a3a44359e9 
>   dbus/dbusmixsetwrapper.cpp 63012d7cbc1e6d416ca59535aa0b0847e6569bab 
>   gui/dialogselectmaster.cpp ed2d8042546f15244097969101c94355ddc53eab 
>   gui/dialogviewconfiguration.cpp 7a5f7fcf6203ba64f281e8804080ee50d3b26aef 
>   gui/kmixdockwidget.cpp ad8d21b73d177535540115013a11fa65b931f09e 
>   gui/mdwenum.h 82d3707bce50715fbf68a34297a243742aa3a0f2 
>   gui/mdwenum.cpp 598aecb3a1a4a00fba81be22a6c8ab3391cacd50 
>   gui/mdwmoveaction.h acc6e6e467df12b92e41e9e5fbb3b2de37e31fd8 
>   gui/mdwmoveaction.cpp 4fafdf1b381ae1825cea54429252bfa13688df4d 
>   gui/mdwslider.h a9b056f0cdebe7bfd184b26685385182cdb4bd1f 
>   gui/mdwslider.cpp 761e667ba5b1a6cd0406230dfc18558d827c755a 
>   gui/mixdevicewidget.h d80d9d899648c0eb615985f411785db1d7e196b0 
>   gui/mixdevicewidget.cpp ae5be25e6fae626da870824adabb9c9b0e229eae 
>   gui/osdwidget.cpp 72b8d9c6ca773831a31f26a4a72537b83949b612 
>   gui/viewbase.h 9ef64bb30d3c0d433b8b94fba11da2f46b73a996 
>   gui/viewbase.cpp 86e503c04a5889351cefe4f43ed6541ff8a76525 
>   gui/viewdockareapopup.h 4201a54c965b8e8457b148e844ff74aca40597b4 
>   gui/viewdockareapopup.cpp cefd8d158e9060fe4d3c4b49715ea65a5c1a75f0 
>   gui/viewsliders.h 707281179955cc4bd9f0891f14ac88ff2621dbda 
>   gui/viewsliders.cpp c8c9f78eb1375c860c38f2bb28b32945b7ecea06 
> 
> Diff: http://git.reviewboard.kde.org/r/111421/diff/
> 
> 
> Testing
> -------
> 
> KMix still seems to work fine, and compilation with libc++ succeeds.
> 
> 
> Thanks,
> 
> Raphael Kubo da Costa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20130901/bdb31763/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia