Peter Dimov wrote:
Michael Glassford wrote:
Peter Dimov wrote:
[...]
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.
I'm not sure where you think the two examples differ. Both attempt a non-blocking lock.
It doesn't really matter, but if you're talking about the way Boost.Threads works currently (in the 1.31.0 release), according to both the code and the documentation, TryLock(m, false) makes no attempt to lock. If you're talking about a new design, then of course it means whatever you want :) That's one reason I prefer the enum: it's clear what the parameter actually means.
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.
Why is that an improvement?
It seems reasonable that, if you're going to allow constructors to lock, you should allow them to lock in whatever ways are possible. I guess I don't feel that strongly about the issue, though; if a single constructor is strongly preferred by others, I guess that's OK with me. However, the constructors I suggested, especially as simplified by Howard, don't seem all that complicated to me. Also, see my comment below.
3) I'm proposing the use of enums rather than bools to indicate the initial state of the lock for these reasons: [...]
OK, let's leave the enum vs bool distinction aside for a while. The crucial difference is explicit try_lock() vs implicit try_lock (a constructor with the appropriate combination of arguments).
OK. I do generally like the idea of simplifying, and a single constructor is certainly simpler. The constructor locking is kind of nice for this syntax, though: if (try_lock l(...)) //or timed_lock { //do something }
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?
My proposal (I'm not sure whether I should be credited for it; Howard posted locks that had these properties and I merely commented) can be made to break such code at compile time, simply by removing the nested TryMutex::scoped_try_lock typedef (as there is no longer a need for it, because ::scoped_lock is a TryLock or TimedLock, as appropriate).
OK. I have to admit I didn't notice this part of the design. I'll have to go back and look again. Mike