Few questions on threading

[Christophe de Vienne <cd...@alphacent.com>]

Hi,

I'm currently working on the threading wrappers, mainly hidding the 
implementation details dependant from the thread system we use.

I have a few questions :
1) In Thread::~Thread(), the join is done directly instead of using 
Thread::join, which make code duplication. Is there a particular reason 
for that ?

2) Thread::thread is protected. Since I'm hidding it, the children 
classes won't be able to access it directly.
The way I hide is the following :
In thread.h :
class Thread {
   // ...
   protected:
      struct Impl;
      Impl const & getImpl();

   private:
      Impl impl;
};

In thread.cpp:
     31 struct Thread::Impl
     32 {
     33     Impl(): thread(0) {};
     34
     35     /** Thread descriptor */
     36 #ifdef LOG4CXX_HAVE_PTHREAD
     37     pthread_t thread;
     38 #elif defined(LOG4CXX_HAVE_MS_THREAD)
     39     void * thread;
     40 #endif
     41
     42 };

This mean that children classes wanting to access the underlying 
descriptor need to know the Impl struct.
So what I would propose is to put the Impl declaration is a separate 
file, threadimpl.h, which the children class implementation file can 
include.

Pre-question is : Does this solution looks good to you ?

More important question : Is it really necessary to have such a 
possibility ?
I mean the abstraction should provide everything needed, and I don't see 
a real case in which it's necessary. Moreover doing this will make more 
complicated to elimate those damn macros we're trying to get rid of.

I have the same question for threadspecificdata and CriticalSection.



More questions may comes later :-)

Regards,

Christophe

Re: Few questions on threading

[Christophe de Vienne <cd...@alphacent.com>]

Christophe de Vienne wrote:

>
> CriticalSection initialise a recursive mutex. For most use it's OK, 
> but some people may need a simple one for performances issue, and 
> there is a third one which can report errors.
> What I propose is to give the type of mutex we need to the 
> constructor, with a default to recursive so it won't break code.
>

Little precision : I can implement this easily with posix thread, but I 
have no idea how to do this with the win32 api. So if someone can have a 
look once I commit it that would be cool :-)

Regards,

Christophe

Re: Few questions on threading

[Christophe de Vienne <cd...@alphacent.com>]

Michaël CATANZARITI wrote:

> Hi,
>
> Christophe de Vienne wrote:
>
>> Hi,
>>
>> I'm currently working on the threading wrappers, mainly hidding the 
>> implementation details dependant from the thread system we use.
>>
>> I have a few questions :
>> 1) In Thread::~Thread(), the join is done directly instead of using 
>> Thread::join, which make code duplication. Is there a particular 
>> reason for that ?
>
>
> You're right Thread::join must be directly called.
> However I think I did not call it directly because of a deadlock under 
> Windows.


Shall I give it a try ? Someone will have to test to me.

>> [snip question about giving access to Impl]
>
> I don't think so. Thread overrides must be platform independant, so 
> the Impl struct has not be known.


OK. It's almost done.

>> More questions may comes later :-)
>

And here it comes !


CriticalSection initialise a recursive mutex. For most use it's OK, but 
some people may need a simple one for performances issue, and there is a 
third one which can report errors.
What I propose is to give the type of mutex we need to the constructor, 
with a default to recursive so it won't break code.


Regards,

Christophe

Re: Few questions on threading

[Michaël CATANZARITI <mc...@apache.org>]

Hi,

Christophe de Vienne wrote:
> Hi,
> 
> I'm currently working on the threading wrappers, mainly hidding the 
> implementation details dependant from the thread system we use.
> 
> I have a few questions :
> 1) In Thread::~Thread(), the join is done directly instead of using 
> Thread::join, which make code duplication. Is there a particular reason 
> for that ?

You're right Thread::join must be directly called.
However I think I did not call it directly because of a deadlock under Windows.

> 
> 2) Thread::thread is protected. Since I'm hidding it, the children 
> classes won't be able to access it directly.
> The way I hide is the following :
> In thread.h :
> class Thread {
>   // ...
>   protected:
>      struct Impl;
>      Impl const & getImpl();
> 
>   private:
>      Impl impl;
> };
> 
> In thread.cpp:
>     31 struct Thread::Impl
>     32 {
>     33     Impl(): thread(0) {};
>     34
>     35     /** Thread descriptor */
>     36 #ifdef LOG4CXX_HAVE_PTHREAD
>     37     pthread_t thread;
>     38 #elif defined(LOG4CXX_HAVE_MS_THREAD)
>     39     void * thread;
>     40 #endif
>     41
>     42 };
> 
> This mean that children classes wanting to access the underlying 
> descriptor need to know the Impl struct.
> So what I would propose is to put the Impl declaration is a separate 
> file, threadimpl.h, which the children class implementation file can 
> include.
> 
> Pre-question is : Does this solution looks good to you ?
> 
> More important question : Is it really necessary to have such a 
> possibility ?

I don't think so. Thread overrides must be platform independant, so the Impl struct has not be known.
> I mean the abstraction should provide everything needed, and I don't see 
> a real case in which it's necessary. Moreover doing this will make more 
> complicated to elimate those damn macros we're trying to get rid of.
> 
> I have the same question for threadspecificdata and CriticalSection.

Same answer !
> 
> 
> 
> More questions may comes later :-)
> 
> Regards,
> 
> Christophe
> 
> 
> 

Regards,

-- 
Michaël CATANZARITI
log4cxx project manager

	log4cxx user mailing list:
	mailto:log4cxx-user-subscribe@logging.apache.org

	log4cxx developer mailing list:
	mailto:log4cxx-dev-subscribe@logging.apache.org