<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/118739/">https://git.reviewboard.kde.org/r/118739/</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 17th, 2014, 4:50 p.m. UTC, <b>Matthew Dawson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line52" style="color: black; font-weight: bold; text-decoration: underline;">src/core/ksharedconfig.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">template</span> <span class="o"><</span><span class="k">typename</span> <span class="n">T</span><span class="o">></span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why create a template function here, when the T must be GlobalSharedConfigList? Why not just put the logic in globalSharedConfigList?</pre>
</blockquote>
<p>On June 18th, 2014, 10:44 p.m. UTC, <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;">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...</pre>
</blockquote>
<p>On June 19th, 2014, 1:10 a.m. UTC, <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;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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....
</pre>
<br />
<p>- David</p>
<br />
<p>On June 21st, 2014, 9:37 a.m. UTC, 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 21, 2014, 9:37 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;">Make KSharedConfig thread-safe
... by having a different list of shareable objects per thread.
REVIEW: 118739</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;">helgrind kiocore-threadtest (not committed yet)
no races detected in KSharedConfig anymore.</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>src/core/ksharedconfig.h <span style="color: grey">(03f05b08a986528798a8eb1c3ead84988e246d4e)</span></li>
<li>src/core/ksharedconfig.cpp <span style="color: grey">(f4b4c766fe19fac92a0651ecc55a89ec3b7636b0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118739/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>