> On June 17, 2014, 12: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...
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. > On June 17, 2014, 12:50 p.m., Matthew Dawson wrote: > > src/core/ksharedconfig.cpp, line 37 > > <https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line37> > > > > Should the KSharedConfig not all be synced to disk before its thread is > > destroyed? Reading QThreadStorage's documentation, the sync can probably > > just happen in the deconstructor, as long as the thread isn't the main > > thread (or maybe just that qApp is still valid). > > David Faure wrote: > KConfig already syncs in the destructor. For secondary threads, it should > therefore already happen automatically, and this code takes care of the main > thread explicitly. > > So I'm not sure what you see as missing here. I didn't think about the destructor. On second thought, the only problematic case is if someone has a thread run with joining with the main thread before the application quits, but I'm not sure if that is something to worry about. I think for now that case isn't worth the effort, and if it does become a problem a proper solution can be created then. - Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118739/#review60298 ----------------------------------------------------------- On June 14, 2014, 5:20 a.m., David Faure wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118739/ > ----------------------------------------------------------- > > (Updated June 14, 2014, 5:20 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. > > > Diffs > ----- > > 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
