Adding ThreadWeaver libraries to kdelibs

[Frans Englich]

On Wednesday 06 September 2006 17:01, Simon Hausmann wrote:
> On Tuesday 5. September 2006 21:35, Mirko Boehm wrote:
> > Hi folks,
> >
> > I think ThreadWeaver is ready for prime time. If you haven't followed it:
> > ThreadWeaver is a job queue that had advanced capabilities for automatic
> > scheduling of application operations. As far as I know, kdevelop, koffice
> > and kdepim are using it or plan to use it as soon as it is commonly
> > available.
> >
> > ThreadWeaver is developed in kdenonbeta at the moment, and usually
> > snapshot and point-released when it reaches a certain new useful state.
> > It is tested, known to be stable, and comes with a set of unit tests.
> >
> > I would like to suggest to add the two main libraries - the Weaver
> > library that contains the job queue and surrounding classes, and the
> > WeaverGui library that holds everything that depends on QtGui, and the
> > accompanying unit tests, into kdelibs. I guess it will be best to import
> > the snapshot point releases into kdelibs, to maintain a state where
> > everything works and is tested, and update as needed. I will take care of
> > the updating.
> >
> > As for the rest, I would like to move it to kdesdk. Unstable versions
> > should be kept and tagged and merged as needed.
> >
> > Please let me know what you think about it.
>
> Looks great! A few minor API suggestions though:
>
> JobCollection::jobListLength() -> jobCount(). Count is the common term used
> in Qt for numbering independent objects in a container. It also goes nicely
> with jobAt(i). Length on the other hand is used for things like
> QLineF::length.
>
> It looks like some functions lack const: JobCollection::canBeExecuted(),
> *Policy::canRun(), hasUnresolvedDependencies().
>
> ResourceRestrictionPolicy::cap: Lacks a getter, and I suggest a different
> name. Perhaps jobLimit or maximumJobCount. (compare leo:cap with leo:limit
> :)
>
> Weaver{Interface}: I suggest to replace *NumberOfThreads with simply
> maximumThreadCount(), threadCount().
>
> For the simplicty of the API (less classes) I also suggest to merge
> WeaverObserver with WeaverInterface, since WeaverObserver does not provide
> any functionality but just signals.
>
> Also with WeaverThreadGrid I suggest to put all member variables into a
> d-pointer.
>
> Job has isFinished() but also bool success(), which looks a bit ugly IMHO.
> Maybe it would be simpler to have a non-virtual
> exitStatus()/setExitStatus() property instead of the virtual success
> function. That's more extensible (with an enum instead of a virtual
> function that cannot be changed later) and it reads nicely:
>
> if (job->isFinished() && job->exitStatus() == Job::ExitSuccess)
>
> And perhaps isFinished() should just be called finished(). That's used more
> often in Qt these days than isFinished().

Hehe, I now remember that I sent a shallow API review to Mirko too. The 
comments are remarkably similar. I remember that some of these were dealt 
with, but it's nevertheless re-pasted below.

-------------------------------------------------------------------------
* Is it intentional that WeaverInterface::{isEmpty,isIdle} aren't const? (if a 
certain sub-class needs to modify itself it should perhaps declare those 
members as mutable) Same with Weaver::id(), WeaverInterface::noOfThreads(), 
and WeaverImpl::queueLength() 

* Perhaps WeaverInterface::noOfThreads() is a bit tricky name. How about 
"threadCount" or even just "count"?

* Perhaps one could rename WeaverInterface::requestAbort() to simply abort(). 
I think it would be simpler, and more consistent with the functions finish(), 
resume(), suspend(), etc. Yes, whether they are aborted is not guaranteed, 
but must the caller be aware of that every time? (I frankly don't know! Just 
throwing out the idea. Many times consistency is better than being 100% 
correct in one particular case).

Some other tips/ideas:

* Break out @code/@endcode code from your Doxygen comments with @include. In 
this way you can compile your code and be *guranteed* they don't contain 
errors. In my code, I even include the code into a QTestLib test[1] and test 
the examples, to the extent possible. You can also use @dontinclude in order 
to only include portions[2]. See the Doxygen manual for getting the 
EXAMPLE_PATH right.

* I see some C++ issues which probably are of interest to fix, especially 
since ThreadWeaver will end up in kdelibs. The easiest way to fix this, is to 
enable a set of defines/GCC warnings. I've attached my Makefile.global which 
I use for KDOM's XQuery/XSL-T code.

* You have very nice example code(the Examples/ stuff). I think it could be 
even better by tying it into the Doxygen. See the @example tag(it will build 
a list, similar to the TODO list, of your examples). You can also describe 
files with the @file tag.
* I would avoid placing function implementations in the header file, unless 
they intentionally should be inlined(and therefore should have the 'inline' 
keyword). The motivation is primarily that the compiler might otherwise 
decide to inline and therefore cause BC trouble(also applies to virtual 
functions, AFAIK). Another motivation is that the headers are parsed faster 
and are easier to read for users. Job::{requestAbort,isFinished} are two 
examples.
* JobCollection, Thread, WeaverImpl have private members and functions. They 
should be placed in a FooPrivate class(kdelibs policy). Don't worry about 
speed ;-) Measures has shown the impact is very small(it's done all over Qt, 
etc).
-------------------------------------------------------------------------


Cheers,

		Frans