<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118985/">https://git.reviewboard.kde.org/r/118985/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 28th, 2014, 11:31 p.m. EDT, <b>Matthew Dawson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not sure about moving the warning about immutable files to only happen on the main thread, as it is possible that an application may use it KSharedConfig on an alternate thread only.
As this is mainly an optimization, I think the best thing is to just move to using the load() function on the atomic integer, as I believe that doesn't require processors to synchronize, and won't limit optimizations. Otherwise, I'm fine with the immutable files as that is simpler code wise. Thoughts?</pre>
</blockquote>
<p>On June 29th, 2014, 9:29 a.m. EDT, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, I'm not sure we'd expect secondary threads to pop up a dialog though (as KConfig::isConfigWritable(true /*warn user*/) does, via QProcess("kdialog")).
This bit (the warnUser code) isn't really about an optimization, but about doing that stuff only in the main thread, where GUI apps initialize the main config object and where user-interaction is expected.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I wouldn't either, and I'd except most code access its main config on a primary thread first. I'm just thinking for those 1% of cases, that cause all the trouble.
When I mentioned about an optimization, I meant is that the reason for this RR, not the point of the user warning. With the relaxed loading of the atomic int, I don't think there is much, if any of a performance hit. I don't think the code complexity is too bad to handle that 1% either. Is there any other reason for this change? Otherwise, I'd prefer to be safe.
Otherwise, this patch looks good with the latest changes.</pre>
<br />
<p>- Matthew</p>
<br />
<p>On June 29th, 2014, 9:55 a.m. EDT, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Matthew Dawson.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated June 29, 2014, 9:55 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KSharedConfig: move mainConfig and wasTestEnabled to the thread storage.
This enables the mainConfig optimization in all threads,
and ensures the user warning only happens in the main thread.
The test-mode-enabled logic is only really useful in the main thread,
but it's simpler to just do it in all threads.
REVIEW: 118985</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">no regression in "make test" in kconfig; still debugging races in helgrind threadtest (in kio).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/kconfigtest.cpp <span style="color: grey">(a8482b7099df5921909830082d758dc3095e3241)</span></li>
<li>src/core/ksharedconfig.cpp <span style="color: grey">(b7d155d5893502921d35d7dd971188b6a93a0620)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118985/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>