Adding ThreadWeaver libraries to kdelibs

[Mirko Boehm]

On Wednesday 06 September 2006 19:01, Simon Hausmann wrote:
> > 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:
Thanks. Nice feedback. 
>
> 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.
Makes sense. In fact, the whole listlength/jobat has only be added to be able 
to access the jobs in derived classes without exporting how they are stored. 
In times of pimples, I may move the two methods in the private class  and 
have the private classes inherit the base class private.
>
> It looks like some functions lack const: JobCollection::canBeExecuted(),
> *Policy::canRun(), hasUnresolvedDependencies().
Possibly. 

Job::canBeExecuted() is not const, it can only be called for Jobs that will be 
executed if it returns true. By definition, executing a Job is not a const 
operation (I think this assumption makes sense). So you have to have a 
non-const object to call canBeExecuted. To make this more clear, there is an 
option to make it private and make Thread a friend of Job, but I do not like 
that idea. 

QueuePolicy::canRun() and derived are not const, because it they are part of 
the canRun/{release,free} protocol that is used to implement queue policies. 
Shortly put, it allocates a queue policy resource (which is an abstract 
concept and depends on what policy is implemented). Release and free later 
free this resource. 

hasUnresolvedDependencies is const now. 
>
> ResourceRestrictionPolicy::cap: Lacks a getter, and I suggest a different
> name. Perhaps jobLimit or maximumJobCount. (compare leo:cap with leo:limit
> :)
OK, getter added. About the name - I think cap is very descriptive. But this 
may be too much focused on US english. Could some native speaker comment on 
that?

>
> Weaver{Interface}: I suggest to replace *NumberOfThreads with simply
> maximumThreadCount(), threadCount().
I did not do that because "thread count" as a term is used to describe the 
quality of bed sheets and linens, and this just sounds weird to me. 
>
> 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.
In fact, this may be a misunderstanding. 

WeaverObserver is mainly used to extend existing implementations. To improve 
the independence of the interfaces, WeaverInterface does not export any hooks 
to implement a WeaverObserver, it only provides a method to add one (and then 
guarantees to call the observers functions as necessary). Since how observers 
are notified by the queue is an implementation detail, I really would prefer 
not to declare any of that in the interface. 

The fact that it only provides signals does not mean that WeaverInterface 
should have these signals. The signals are a convenience, the actual 
implementation could as well be in the derived WeaverObserver class. 
>
> Also with WeaverThreadGrid I suggest to put all member variables into a
> d-pointer.
Please see the other mail about the WeaverGui library. 

>
> 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:
Th real issue here is that success is not a generic enough concept to be held 
in a member variable. Most operations that are done in Jobs have a natural 
concept of success. Therefore, a generic setExitStatus method is bound to be 
hard to implement (the state may not even be settable if you interface an 
existing implementation of an operation). Also, an enum is not extendable. 
So possibly it makes sense to simply have success() the way it is, and add 
domain-specific ideas of success to derived job classes. But this is 
definitly open for discussion. Maybe blackarrow can comment on it, he seems 
to use that stuff in kdevelop. 

Generally, I found it rather hard to add generic concepts to Job. Success is 
not generic. Return values are not generic. Progress is a bit generic (this 
is one of the features I think about adding). Using virtual methods allows to 
tie the actual job implementation to it. I would expect that by using a value 
member for it, users are forced to work around that implementation if it does 
not fit their needs. 

>
> 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().
Yes, that is quite some inconsistency in Qt :-) QWidget has 13 methods that 
start with "is", IIANM. But I will think about it. 

Thanks, 
--Mirko.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 186 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20060907/972eac59/attachment.sig>