Adding ThreadWeaver libraries to kdelibs

[Simon Hausmann]

On Thursday 07 September 2006 07:58, Mirko Boehm wrote:
[...]
> > 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.

Ahh, ok.

> 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.

Hm, I see.

> > 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?

I asked Zack and he says that the API is not used by native speakers only and 
he prefers something more descriptive.

Qt uses cap only in QRegExp for captures and for QPen::capStyle. Maximum or 
limit on the other hand is used a lot more often.

Either way I think some prefix or suffix can't hurt.

> > 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.

Well, numberOf* is a pattern that is not used at all in Qt or KDE. And the 
non-native speaker argument applies here, too :). For them (me) consistency 
is more important I think.

> > 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.

I'm not sure I understand...

WeaverObserver is neither an interface nor does it provide any functionality. 
What use is a generic way of adding observers to your weaver interface if any 
use of it is implementation specific anyway? Why not provide simple and 
convenient signals in the Weaver to catch the 99% use-case?

> > 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.

It's not the is pattern I'm referring to, it's 'isFinished' versus 'finished', 
in combination with 'success'. The terms are related but their naming appears 
inconsistent right now.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20060907/52aaf67e/attachment.sig>