Review Request 118985: KSharedConfig: only write to mainConfig and wasTestModeEnabled in the main thread

[Mark Gaiser]

> On June 27, 2014, 11:47 p.m., Mark Gaiser wrote:
> > src/core/ksharedconfig.cpp, line 91
> > <https://git.reviewboard.kde.org/r/118985/diff/1/?file=285018#file285018line91>
> >
> >     -- slightly unrelated since it wasn't there before this patch.
> >     
> >     } else {
> >         wasTestModeEnabled = false;
> >     }
> >     
> >     ?
> >     
> >     I don't know this code, but what happens here is wasTestModeEnabled is initialized to false, then set to true if it's false and isTestModeEnabled is true. Fine, but it's never set back to false again.
> >     
> >     I don't know if it's an issue at all.. Just something i noticed.
> >     
> >     The same for the "userWarned" a few lines down.
> 
> David Faure wrote:
>     Unittests can enable test mode, and are not supposed to ever disable it again. The whole point is to protect the user's real config files, so it wouldn't really make sense to disable test mode again.
>     
>     "userWarned" is also a one-way street. Once the user has been warned, userWarned is true. You can't change the past and make it false again :)

Ahh, now it makes sense. Thank you for explaining.


- Mark


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


On June 27, 2014, 10:31 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118985/
> -----------------------------------------------------------
> 
> (Updated June 27, 2014, 10:31 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> KSharedConfig: only write to mainConfig and wasTestModeEnabled in the main thread
> 
> As a side effect, this removes the need for the atomic ints, since
> that code now only runs in the main thread.
> 
> 
> Diffs
> -----
> 
>   src/core/ksharedconfig.cpp b7d155d5893502921d35d7dd971188b6a93a0620 
> 
> Diff: https://git.reviewboard.kde.org/r/118985/diff/
> 
> 
> Testing
> -------
> 
> no regression in "make test" in kconfig; still debugging races in helgrind threadtest (in kio).
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140628/372b8c8e/attachment.html>