<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/107082/">http://git.reviewboard.kde.org/r/107082/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 29th, 2012, 11:18 a.m., <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;">This assumes that QRegExp::exactMatch is re-entrant, i.e. that it doesn't update an internal cache on demand. I'm not so sure of that, the regexp parsing sounds like a very good candidate for on-demand parsing, unless you make extra sure that it's all done upfront.</pre>
</blockquote>
<p>On October 29th, 2012, 4:34 p.m., <b>Simeon Bird</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;">The Qt documentation claims it is:
http://qt-project.org/doc/qt-4.8/qregexp.html
"Note: All functions in this class are reentrant."
(although it's true that I hadn't checked up to now, so that was still a good question)</pre>
</blockquote>
<p>On October 29th, 2012, 5:26 p.m., <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;">Never trust documentation :-)
I had a look at the source code, and found indeed
1) on-demand initializations
2) no locking mechanism.
So I wrote a test program, and guess what? It gives different output from one run to the next. It's memcheck-clean, but helgrind manages to spot the issue in some runs.
http://www.davidfaure.fr/2012/qregexp_race.cpp
http://www.davidfaure.fr/2012/qregexp-helgrind-log
The program is supposed to write out "true true true" but only does so 1 time out of 8 on average, in my tests. Nice, heh?
=> The documentation is wrong. I'll submit a docu fix to Qt...</pre>
</blockquote>
<p>On October 29th, 2012, 5:31 p.m., <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;">Wait, the documentation is right, the class is reentrant (i.e. different instances can be used in different threads).
It's just not threadsafe (i.e. you can't use the same instance of QRegExp in different threads).
Therefore the documentation isn't wrong, but this patch is.
You must use one QRegExp per thread, or ensure exclusive use of the QRegExp instance.
A QThreadStorage would make it possible to use one QRegExp per thread, but is rather difficult to handle if the regexp could change at runtime. So I think the only solution is "exclusive use of the QRegExp instance", i.e. QWriteLocker rather than QReadLocker (new conclusion: careful with what appears to be readonly).</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;">Aha! Ok, you're quite right. I see also that I was confused about the difference between POSIX's definition of re-entrant and Qt's...
I'll make it a WriteLocker and add a comment in v2. Thanks for teaching me C++ once again :)</pre>
<br />
<p>- Simeon</p>
<br />
<p>On October 27th, 2012, 6:29 p.m., Simeon Bird wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated Oct. 27, 2012, 6:29 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;">Some fairly trivial misc improvements to the filewatch service. Probably don't make a big difference, but probably a good idea.
- Use QReadWriteLock instead of QMutex in FileIndexerConfig, thus allowing multiple threads to call shouldFolderBeIndexed at once (not that we really do that right now).
- Add the IN_EXCL_UNLINK inotify flag. On recent (2.6.36) kernels, this means we don't generate events for files once
they have been unlinked from the directory we are watching. Prevents waking up for some already-deleted temporary files.
</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 for a bit, didn't seem to break anything. </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>services/fileindexer/fileindexerconfig.h <span style="color: grey">(7debaf3)</span></li>
<li>services/fileindexer/fileindexerconfig.cpp <span style="color: grey">(5226a79)</span></li>
<li>services/filewatch/kinotify.h <span style="color: grey">(6e3f1c0)</span></li>
<li>services/filewatch/kinotify.cpp <span style="color: grey">(9868b90)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107082/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>