> On Oct. 29, 2012, 11:18 a.m., David Faure wrote: > > 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. > > Simeon Bird wrote: > 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) > > David Faure wrote: > 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... > > David Faure wrote: > 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). > > Simeon Bird wrote: > 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 :) > > Sergey Borovkov wrote: > What do you mean difference in definition? If I am not mistaken > re-entrant function can't use global data - and you are using just that. All > class members have explicit 'this' parameter passed to to function. > > Simeon Bird wrote: > Qt docs say: > "A reentrant function can also be called simultaneously from multiple > threads, but only if each invocation uses its own data." > which QRegExp::exactMatch is documented to be, and is, as David pointed > out. > > POSIX says: > A "reentrant function" is defined as a "function whose effect, when > called by two or more threads, is guaranteed to be as if the threads each > executed the function one after another in an undefined order, even if the > actual execution is interleaved" > (http://www.unix.org/whitepapers/reentrant.html) > > which it is not, no? Due, as you say, to the use of the 'this' parameter. > > I thought the Qt docs were implying the latter when they really only > meant the former.
Yes, re-entrant C functions don't have the notion of "modifying the object pointed by the first argument" (that would make them not re-entrant), so indeed it's not exactly the same. In Qt (C++) most methods are reentrant but not threadsafe - i.e. you need to use a different instance. It's all a matter of definition indeed. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107082/#review21082 ----------------------------------------------------------- On Oct. 27, 2012, 6:29 p.m., Simeon Bird wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107082/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2012, 6:29 p.m.) > > > Review request for Nepomuk, Vishesh Handa and Sebastian Trueg. > > > Description > ------- > > 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. > > > Diffs > ----- > > services/fileindexer/fileindexerconfig.h 7debaf3 > services/fileindexer/fileindexerconfig.cpp 5226a79 > services/filewatch/kinotify.h 6e3f1c0 > services/filewatch/kinotify.cpp 9868b90 > > Diff: http://git.reviewboard.kde.org/r/107082/diff/ > > > Testing > ------- > > Compiled, ran for a bit, didn't seem to break anything. > > > Thanks, > > Simeon Bird > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
