[boost] Re: Boost.Threads: Do we need all those mutexes?

From: Michael Glassford (glassfordm_at_[hidden])
Date: 2004-07-07 11:30:19

Peter Dimov wrote:
> Michael Glassford wrote:
> 
>>What about the following as an alternative? I retain the hierarchy
>>(a TryLock is a Lock, a TimedLock is a TryLock) and resolve the conflict
> 
> (of
> 
>>consistency within a concept and consistency across concepts) by the
>>principle that a constructor always blocks unless you specifically tell it
> 
> not to:
> 
>>Definitions
>>-----------
>>M: appropriate mutex type
>>
>>namespace lock_state {
>>     typedef enum
>>     {
>>         unlocked=0,
>>         locked=1
>>     } lock_state;
>>} //namespace lock_state
>>
>>namespace read_write_lock_state {
>>     typedef enum
>>     {
>>         unlocked=0,
>>         read_locked=1,
>>         write_locked=2
>>     } read_write_lock_state;
>>} //namespace read_write_lock_state
>>
>>namespace blocking_mode {
>>     typedef enum
>>     {
>>         non_blocking=0,
>>         blocking=1
>>     } blocking_mode;
>>} //namespace blocking_mode
>>
>>ScopedLock / ScopedReadLock / ScopedWriteLock
>>---------------------------------------------
>>     lock(M) //always locking, blocking
>>     lock(M, lock_state) //blocking
>>
>>ScopedTryLock / ScopedTryReadLock / ScopedTryWriteLock
>>------------------------------------------------------
>>     try_lock(M) //always locking, BLOCKING
>>     try_lock(M, lock_state) //BLOCKING
>>
>>     try_lock(M, lock_state, blocking_mode)
>>     try_lock(M, blocking_mode) //always locking
>>
>>
>>ScopedTimedLock / ScopedTimedReadLock / ScopedTimedWriteLock
>>------------------------------------------------------------
>>     timed_lock(M) //always locking, blocking
>>     timed_lock(M, lock_state) //blocking
>>
>>     timed_lock(M, lock_state, blocking_mode)
>>     timed_lock(M, blocking_mode) //always locking
>>
>>     timed_lock(M, t) //always locking, blocking for time t
> 
> 
> This is certainly possible, but I don't see what the additional complexity
> buys us.
> 
>     TryLock l( m, false );
> 
>     if( l.try_lock() )
>     {
>     }
> 
> looks acceptable to me.
> 
>     TryLock l( m, non_blocking );
> 
>     if( l.locked() )
>     {
>     }
> 
> doesn't seem much of an improvement.

The two aren't equivalent (I'm not sure if you meant them to be): in 
your first example the constructor doesn't attempt to lock, and your 
second example it attempts a non-blocking lock.

In any case,

1) What it buys over the current design:

* Consistent behavior across lock types (which isn't true now: 
"lock_type l(M)" currently is blocking for Lock, non-blocking for 
TryLock, and doesn't exist for TimedLock AFAICS).

* Consistent behavior within a lock type (which isn't true now: 
"try_lock l(M)" is non-blocking, while "try_lock l(M, true)" is 
blocking, which is rather arbitrary). The consistent behavior I mean is 
that constructors not taking a blocking_mode parameter always block.

* The ability to specify all applicable lock types (blocking, 
non-blocking, timed-blocking) in a constructor.

2) What it buys over your suggestion (at the cost of complexity):

* The ability to specify all applicable lock types (blocking, 
non-blocking, timed-blocking) in a constructor.

3) I'm proposing the use of enums rather than bools to indicate the 
initial state of the lock for these reasons:

* Enums are more explicit, which is good.

* Consistency with read/write locks (if we allow such critters): they 
can't use a bool because they have three states.

* Having said that, now that I've changed my mind about what try_lock(M, 
true) should do (I now think it should block), my primary original 
motivation for using enums is no longer true: it was to prevent the 
meaning of try_lock(M, true) from changing silently. So I'm not opposed 
to continuing to use a bool if people feel strongly about it, or to 
providing both an enum and a bool version (possibly only temporarily).

4) Which leads to a problem with both your proposal and mine: they both 
silently break existing code (by changing try_lock(M) from non-blocking 
to blocking); at least, I presume you meant that. Would eliminating the 
lock_type(M) constructor (perhaps only for a release or two) be a good 
idea, or is a prominent warning in the documentation enough?

Mike