<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="http://git.reviewboard.kde.org/r/109747/">http://git.reviewboard.kde.org/r/109747/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for picking this up again, I like where it's going overall. Found one refactoring bug though, plus I have a few questions.</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109747/diff/1/?file=122256#file122256line331" style="color: black; font-weight: bold; text-decoration: underline;">libnepomukcore/resource/resourcedata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool Nepomuk2::ResourceData::store()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">322</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_rm</span><span class="o">-></span><span class="n">addToWatcher</span><span class="p">(</span> <span class="n">m_uri</span> <span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">323</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_rm</span><span class="o">-></span><span class="n">addToWatcher</span><span class="p">(</span> <span class="n">m_uri</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is there a risk another thread could modify m_uri at this point? Or are we sure it's never empty when this is called?</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109747/diff/1/?file=122256#file122256line454" style="color: black; font-weight: bold; text-decoration: underline;">libnepomukcore/resource/resourcedata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void Nepomuk2::ResourceData::setProperty( const QUrl& uri, const Nepomuk2::Variant& value )</pre></td>
</tr>
</tbody>
<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">435</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">updateKickOffLists</span><span class="p">(</span> <span class="n">uri</span><span class="p">,</span> <span class="n">m_cache</span><span class="p">.</span><span class="n">value</span><span class="p">(</span><span class="n">uri</span><span class="p">),</span> <span class="n">value</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This uses m_cache.value() outside of the m_dataMutex lock.
Plus, at that point, m_cache.value(uri) is always 'value', isn't it? Should this use an "oldValue" local variable instead, set before m_cache is updated, and from within the lock?</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109747/diff/1/?file=122256#file122256line512" style="color: black; font-weight: bold; text-decoration: underline;">libnepomukcore/resource/resourcedata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void Nepomuk2::ResourceData::addProperty( const QUrl& uri, const Nepomuk2::Variant& value )</pre></td>
</tr>
</tbody>
<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">488</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">updateKickOffLists</span><span class="p">(</span> <span class="n">uri</span><span class="p">,</span> <span class="n">oldvalue</span><span class="p">,</span> <span class="n">value</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(this one looks correct, for instance)</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/109747/diff/1/?file=122257#file122257line414" style="color: black; font-weight: bold; text-decoration: underline;">libnepomukcore/resource/resourcemanager.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void Nepomuk2::ResourceManager::setOverrideMainModel( Soprano::Model* model )</pre></td>
</tr>
</tbody>
<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">414</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// to take the mutex anyway.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What do you mean by "take" here? Lock? Surely if the calling function was locking the rm mutex before calling addToWatcher, this line would deadlock... Not sure what this comment means.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On March 26th, 2013, 7:54 p.m. UTC, Simeon Bird wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Nepomuk, David Faure and Vishesh Handa.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated March 26, 2013, 7:54 p.m.</i></p>
<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;">Am I ok to merge this to KDE 4.10? I would like to merge the actual bugfixes,
but if you want I could omit the performance fixes/cleanups, which are
1e76eedb82ee363c9caf333fe221962db81d67c6 75028c00ec71ccd67a046c0d71072623c2e5af74 464590095981e7ded537901d56f84ca49aa49d74
.
Thanks
commit dbbea65fb00c14852034be867396aa055041d848
Author: Simeon Bird <bladud@gmail.com>
Date: Sat Mar 16 19:11:19 2013 -0400
ResourceWatcher: switch to using KDbusConnectionPool
commit 1e76eedb82ee363c9caf333fe221962db81d67c6
Author: Simeon Bird <bladud@gmail.com>
Date: Thu Mar 14 23:17:31 2013 -0400
Tidy up ResourceData::propertyAdded.
If an entry is not found in a QHash, it will return an empty list,
so we only need to check for contains one way.
commit 75028c00ec71ccd67a046c0d71072623c2e5af74
Author: David Faure <faure@kde.org>
Date: Thu Mar 14 14:27:47 2013 +0100
Rework determineUri so that the RM lock isn't held during the executeQuery
commit cc8a925989193b813dc50090c8b4a05e46fbfbdc
Author: David Faure <faure@kde.org>
Date: Thu Mar 14 14:26:45 2013 +0100
fix lock order in resetAll
commit aec508539182f7bed2819cd23d82ba9baa120c8c
Author: David Faure <faure@kde.org>
Date: Thu Mar 14 13:23:22 2013 +0100
early return if nothing to do (noop, just makes the code clearer)
commit 877b40f1916a64916d0869be5744dbc525931edd
Author: Simeon Bird <bladud@gmail.com>
Date: Sat Mar 16 16:27:44 2013 -0400
Fix bad threading in ResourceManagerPrivate::addToWatcher dbus usage.
Instead of doing MoveToThread, take the resourceManager mutex before
talking to the ResourceWatcher. This is a lot clearer and less racy.
It should not be too contended now the rm mutex is not held over the
socket communication.
commit 464590095981e7ded537901d56f84ca49aa49d74
Author: Simeon Bird <bladud@gmail.com>
Date: Fri Mar 8 22:08:17 2013 -0500
ResourceData: Change updateKickOffLists to take three arguments, the
uri, old and new values.
This means that it does not need to be under the dataMutex anymore,
which in turn means that we no longer need to remember to lock the RM
mutex before calling it, just to remember to unlock the dataMutex.
This in turn means that we can add a property that isn't NIE::url() or
NAO::identifier to the Resource without taking the mutex at all.
commit 1b82506d609866e87b9d49b47de473766b01f3d6
Author: Simeon Bird <bladud@gmail.com>
Date: Sat Mar 9 03:25:09 2013 -0500
Remove unneeded parameter from ResourceData::resetAll</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;">Compiled, ran, used activities for a while.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=305024">305024</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libnepomukcore/datamanagement/resourcewatcher.cpp <span style="color: grey">(f394ae8705fa882b9f4329f93ea7d18c1cfabc73)</span></li>
<li>libnepomukcore/resource/resource.cpp <span style="color: grey">(7158802cf1e1ba48da86f103aa9311c7acbe24fe)</span></li>
<li>libnepomukcore/resource/resourcedata.h <span style="color: grey">(39508764682b798290c1ae5b74a2384f9cbcbf58)</span></li>
<li>libnepomukcore/resource/resourcedata.cpp <span style="color: grey">(7a979745ca22c88a3a73921d9c0e64b51e064c38)</span></li>
<li>libnepomukcore/resource/resourcemanager.cpp <span style="color: grey">(2b32be01176059932cc1c2cf3b1f348ed91e982b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109747/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>