ThreadSpecificData improvement.

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

Curt Arnold a écrit :

>
> On Aug 13, 2004, at 9:44 AM, Christophe de VIENNE wrote:
>
>> Any idea/suggestion welcome,
>>
>>
>
> Not directly on point, but I had a couple of random, unconsidered 
> thoughts related to threads that I'd like to throw out if anyone is 
> going to be digging into to this.
>
> 1. Apache Portable Runtime (http://apr.apache.org) provides a C 
> threading API.  Thought it probably isn't desirable to require APR 
> especially just for threading, it might be good to support it as a 
> threading option to support rarer platforms.

The threading API of APR looks quite simple (and close to pthread), it 
shouldn't be a problem to support it the day it's needed.

> 2. The CriticalSection instance in log4cxx::helpers::ObjectImpl causes 
> config.h to be expanded for the abstract classes in public API.  
> Isolating implementation details from the client API would require 
> removing the CriticalSection from the abstract base class and 
> reintroducing it in the concrete implementation classes. perhaps via 
> templating and/or macros.

Another solution is to typedef the main types we use in threading, 
depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
the library user to add -DPTHREAD on it's compiler command line, which 
is not a problem I think (a lot of other library do).
For example in portability.h, having:

#ifdef PTHREAD
typedef pthread_mutex_t log4cxx_mutex_t;
typedef phtread_key_t log4cxx_key_t;
typedef pthread_t log4cxx_thread_t;
#else
#ifdef WIN32
typedef CRITICAL_SECTION log4cxx_mutex_t;
typedef void * log4cxx_key_t;
typedef void * log4cxx_thread_t;
#endif
#warning "No threading support"
#endif



OR, we keep the HAVE_xx macros, but build them from PTHREAD and WIN32 :

#if defined(PTHREAD)
# define LOG4CXX_HAVE_PTHREAD
#elif defined(WIN32)
# define LOG4CXX_HAVE_MSTHREAD
#endif



About TheadSpecificData, I'm thinking of improving the API this way :

class ThreadSpecificData
{
    public:
        ThreadSpecificData();
        ThreadSpecificData(void (*cleanup)(void*));  // this is a new 
constructor.

        void * GetData() const;
        void SetData(void * data);

   // ....
};


/** This ptr interface is taken from boost, I don't know if we can take 
it like this. On the other hand, starting from scratch with the same 
goal in mind, we'd end with the exact same result... */
template < class T >
class thread_specific_ptr
{
    public:
         tsd_ptr(): m_impl( &tsd_ptr::cleanup ) {}

         T* get() const {
              return m_imp->getData();
         }

         T* operator->() const { return get(); }
         T& operator*() const { return *get(); }
        
         T* release() {
             T* tmp = get();
             m_impl.setData(0);
             return tmp;
         }

         operator bool () { return get() == 0; }
 
         operator T& () { return *get(); }

          void reset(T* p=0) {
             T* tmp = m_impl.getData();
             if(tmp)
                 delete tmp;
            impl_.setData(p);
         }


    private:
       static void cleanup(void* p) { delete static_cast<T*>(p); }

        ThreadSpecificData m_impl;
};





Christophe

Re: ThreadSpecificData improvement.

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

Curt Arnold wrote:

>> Another solution is to typedef the main types we use in threading, 
>> depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
>> the library user to add -DPTHREAD on it's compiler command line, 
>> which is not a problem I think (a lot of other library do).
>> For example in portability.h, having:
>
> The mechanism used for critical sections is an implementation detail 
> that is ideally invisible to the client application.  I think it is 
> much more preferable to hide that implementation detail than to make 
> the build for the client application guess the value used to build the 
> library.

Ok then let's go for pimpl. I'll keep you informed.

Cheers,

Christophe

Re: ThreadSpecificData improvement.

[Curt Arnold <ca...@houston.rr.com>]

>> 2. The CriticalSection instance in log4cxx::helpers::ObjectImpl 
>> causes config.h to be expanded for the abstract classes in public 
>> API.  Isolating implementation details from the client API would 
>> require removing the CriticalSection from the abstract base class and 
>> reintroducing it in the concrete implementation classes. perhaps via 
>> templating and/or macros.
>
> Another solution is to typedef the main types we use in threading, 
> depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
> the library user to add -DPTHREAD on it's compiler command line, which 
> is not a problem I think (a lot of other library do).
> For example in portability.h, having:
>

The mechanism used for critical sections is an implementation detail 
that is ideally invisible to the client application.  I think it is 
much more preferable to hide that implementation detail than to make 
the build for the client application guess the value used to build the 
library.