Review Request 118739: Make KSharedConfig thread-safe

[David Faure]

> On June 17, 2014, 4:50 p.m., Matthew Dawson wrote:
> > src/core/ksharedconfig.cpp, line 52
> > <https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line52>
> >
> >     Why create a template function here, when the T must be GlobalSharedConfigList?  Why not just put the logic in globalSharedConfigList?
> 
> David Faure wrote:
>     Because I copied this template into 4 different files in KIO as well. It basically makes this bit of code reuseable elsewhere more easily
>     (but this isn't big enough to be worth being put into Qt, especially since there is a slight variation between the cases where we need to be able to call hasLocalData from the outside, like here, and the cases in KIO where this is not needed and therefore the QThreadStorage instance can be moved into the function).
>     
>     I see your point that when reading just that file, the template thing looks surprising.
>     grep KIO for perThreadGlobalStatic to see a pattern emerging, though...
> 
> Matthew Dawson wrote:
>     Ok, that makes sense.  It seems to me it would good for Qt to have a getOrCreate function on QThreadStorage, as that is all the template function is doing anyways.  But Qt doesn't have such a function, so I think the template can just stay.

Yes but the devil is in the details. Either the template encapsulates a static QThreadStorage (much better in general) or it needs to expose it (like here), and there is the issue of constructor arguments (there are none in our use cases, but there could be, in general). So it's not that simple to "put this simple template into Qt", it's not generic enough.

A Q_THREAD_GLOBAL_STATIC modelled after Q_GLOBAL_STATIC would be nice, in fact. I.e. not a template, but a macro encapsulating a template....


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118739/#review60298
-----------------------------------------------------------


On June 21, 2014, 9:37 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118739/
> -----------------------------------------------------------
> 
> (Updated June 21, 2014, 9:37 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> Make KSharedConfig thread-safe
> 
> ... by having a different list of shareable objects per thread.
> 
> REVIEW: 118739
> 
> 
> Diffs
> -----
> 
>   src/core/ksharedconfig.h 03f05b08a986528798a8eb1c3ead84988e246d4e 
>   src/core/ksharedconfig.cpp f4b4c766fe19fac92a0651ecc55a89ec3b7636b0 
> 
> Diff: https://git.reviewboard.kde.org/r/118739/diff/
> 
> 
> Testing
> -------
> 
> helgrind kiocore-threadtest (not committed yet)
> 
> no races detected in KSharedConfig anymore.
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140621/7d8460a0/attachment.html>