----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18623 -----------------------------------------------------------
A number of comments on the implementation, but also a more general comment: does it even make sense to use a Speller in multiple threads, and to change the language from one thread while another one is using the speller for spell-checking? It sounds like, even if a mutex/lock can prevent crashes, this is a weird thing to do anyway, since you have no idea at which point the spell-checking will switch to another language... could happen in the very middle of a sentence... Maybe it would make more sense to "post" the language change operation to the spell-checking thread using the same mechanism as the one used to "post" spellchecking requests to it? (Disclaimer: I know nothing of the krunner architecture). kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14723> if() before delete is unnecessary, please revert this change. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14726> That line is racy, if called from two different threads (copying a pointer is not atomic). If anything else, "helgrind" will warn for sure. Move it inside the lock. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14727> This line is a no-op, remove it. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14729> const QString& lang = QString() (missing const ref, missing spaces) kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14728> coding style: if (lang.isEmpty()) { (space after if, space before brace). Same thing on other lines below. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14730> If setLanguage is called from another thread as the thread using "lang", then this creates a race on "lang". Protect it with a mutex. More generally, it seems that all member variables here need mutex protection, not just dict. So I'd rename dictLock to lock, and use it for all members. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14724> See above. Delete NULL is perfectly valid (and no-op) in C++. kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14731> spaces around = kdecore/sonnet/speller.cpp <http://git.reviewboard.kde.org/r/106242/#comment14725> Please use QReadLocker instead. This will simplify the code, no need for the 3-liner return at the end of the method. - David Faure On Aug. 27, 2012, 9:33 p.m., Simeon Bird wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106242/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2012, 9:33 p.m.) > > > Review request for kdelibs and Plasma. > > > Description > ------- > > Krunner's spellcheck plugin has been pretty broken since > bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it > called Sonnet::Speller::setLanguage every time the spellchecker was invoked, > which was (very much) not thread-safe. > > This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all > access to the internal dict pointer using QReadWriteLock. > > A related review request is 106244, which adds more fixes to the spellcheck > feature. > > > This addresses bugs 264779 and 303831. > http://bugs.kde.org/show_bug.cgi?id=264779 > http://bugs.kde.org/show_bug.cgi?id=303831 > > > Diffs > ----- > > kdecore/sonnet/speller.cpp b19e74d > > Diff: http://git.reviewboard.kde.org/r/106242/diff/ > > > Testing > ------- > > Compiled, installed, used for a week or so, spellchecked a bunch of things. > > > Thanks, > > Simeon Bird > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
