Apologies that some of this has fallen behind the current discussion: I tried to post it last night, but got errors, and am posting it again today unchanged. Mike Howard Hinnant wrote:
Thanks everyone for your comments and patience. There's a new spec up at:
Sorry I haven't had time to read and comment as well due to the upcoming release; my comments here are pretty brief as well. I hope to be able to do better soon.
http://home.twcny.rr.com/hinnant/cpp_extensions/threads.html
I've tried to incorporate those comments I've agreed with, and explain my disagreements below. Further discussion would be much appreciated. I feel there has been dramatic improvements in this first iteration of the spec. Thanks again!
On Jul 18, 2004, at 6:41 PM, Batov, Vladimir wrote:
1. explicit scoped_lock(mutex_type& m); 2. scoped_lock(mutex_type& m, locking_enum lock_it); 3. scoped_lock(mutex_type& m, blocking_enum block_it); 4. scoped_lock(mutex_type& m, const elapsed_time& elps_time);
Do we need (3)? Isn't is covered by (4) with elps_time = INFINITE (blocking) and - 0 (non blocking)? If it is intended as a "convenience" interface, my expectation would be that average users would use thin and more explicit wrappers around the general lock. Like try_lock and timed_lock. Then, those thin wrappers would address the "convenience" issue better.
I agree that a timed lock collapses to non-locking and non-blocking on the extremes, but I don't think it is a good idea to collapse the implementations or the signatures. Separate constructors allows a clean separation of ideas, and also allows for the fact that the underlying mutex may well support try_lock (for example), but not timed_lock.
I agree with your disagreement.
However, I've modified the signatures of the enum-based constructors (read on).
(1) and (2) overlap as (1) is (2) with lock_it = true. Does not it mean that some functionality will have to be duplicated in (1) and (2)? How about avoiding the overlap with
1. explicit scoped_lock(mutex_type& m); 2. scoped_lock(mutex_type& m, deferred_t);
Then, every constructor would do just one thing.
I agree 100%, though I've spelled deferred_t as defer_lock_type and added try_lock_type. Thanks. You can now:
scoped_lock lk1(m, defer_lock); // not locked scoped_lock lk2(m, try_lock); // tries to lock
If you remember, I proposed something along these lines (at the suggestion of Vladimir, IIRC) and liked it a lot, but changed my mind when it was pointed out that it prevents making the choice at run time whether the lock should be locked or not. With movable locks, as you point out below, this is changed somewhat.
On Jul 19, 2004, at 6:55 AM, Bronek Kozicki wrote:
* do we need distinction between "write_lock" and "scoped_lock"? After all, both lock are: 1. exclusive (in regard to ownership of "mutex" object) 2. moveable (ie. have move semantics instead of copy semantics)
write_lock delivers some superset of functionality that scoped_lock does not have, but do we need separate class for this functionality? After all "if the client of scoped_lock<Mutex> does not use funtionality which the Mutex does not supply, no harm is done". I understand that under current proposal mutex class may deliver three separate families of lock functions : lock, read_lock and write_lock, but I want you to consider just two: lock (== exclusive == write) and shared_lock (== read). I suggest that scoped_lock (another possible name "exclusive_lock") would be move-constructible also from upgradable_shared_lock. If Mutex class does not deliver member function unlock_upgradable_shared_lock_exclusive , we would have compilation error. Otherwise, no harm is done. The same applies to move-assignment from upgradable_shared_lock (hm, some shorter name please?)
Good comments. Here's what I've done in response:
scoped_lock |----------------> scoped_lock write_lock | read_lock -------------------> sharable_lock upgradable_read_lock ---------> upgradable_lock
At first glance, this looks good to me.
* I do not quite like "write_lock(try_rvalue_ref<upgradable_read_lock<mutex_type> > r);" (neither "exclusive_lock(try_rvalue_ref<upgradable_shared_lock<mutex_type> > r)" ). It seems inconsistent with rest of interface. What about "write_lock (rvalue_ref<upgradable_read_lock<mutex_type> > r, blocking_enum block_it);" ?
My problem is that this syntax does not generalize to the try-move-from-upgradable_lock-assignment:
upgradable_lock ul(m);
scoped_lock sl (try_move(ul)); sl = try_move(ul);
I really want the try-to-upgrade syntax to be consistent between copy ctor and assignment forms. The consistency makes the interface easier to learn.
* mutex() memeber function may return void* instead of "const mutex_type*". This will make it suitable just to check for equality of owned mutex, while preventing any kind of mutex manipulation. Just an idea, because "const" (which is already there) in most cases should suffice.
I went with Peter's non-const pointer.
* I'd still like to have number of free functions:
template <typename Mutex> scoped_lock<Mutex> lock_mutex(Mutex); // or "acquire"
template <typename Mutex> scoped_lock<Mutex> try_lock_mutex(Mutex); // nonblocking; or "try_acquire"
template <typename Mutex> scoped_lock<Mutex> try_lock_mutex(Mutex, const elapsed_time&); // or "try_acquire"
These functions might look little different (eg. use enumerations locking_enum and blocking_enum). The idea is that function will create lock appropriate for given Mutex type - assuming that there migh be Mutex-es delivered with their own lock class, or that.
I'm not completely against the free functions. But I do feel that they are syntax sugar for the constructors that must be there anyway (discussed more below). And if you have them for one lock, you need them for all 3 locks for consistency.
* I'd like to have common base class over all lock classes. I do not want polymorphic behaviour (all member functions might be protected, including destructor, ctor and cctor), but to allow following very simple and I believe typical usage scenarios:
The common base class stuff makes me nervous. I don't want polymorphic behavior either. But if it isn't polymorphic behavior what would ~lock() do? Choices are m_.unlock(), m_.unlock_sharable() and m_.unlock_upgradable().
On Jul 19, 2004, at 8:47 AM, Peter Dimov wrote:
I mostly agree with Bronek Kozicki. Given a movable lock, Eric Niebler's proposal:
scoped_lock try_lock( Mutex & m ); scoped_lock timed_lock( Mutex & m );
is a better try/timed interface. Heisenberg constructors must die.
Sorry, I don't know what a Heisenberg constructor is.
One that leaves you uncertain what state it's in after it runs (name taken from the "Heisenberg Uncertainty Principle" of physics).
I don't dislike these helper functions. But I do dislike the idea that they should be the only way to construct-and-try or construct-with-time. The problem is that we have 3 different types of locks, and I want to be able to construct-and-try in a generic-lock function:
template <class Lock> void foo() { Lock lk(m, try_lock); ... }
The above code should work for scoped_lock, sharable_lock and upgradable_lock. I don't see how to make that work with the helper function syntax, at least not without getting a lot uglier (like using explicit template arguments).
I like the free-function syntax, but this seems like a good point to me.
Also, I still think that the bool parameter is better than a "locking" enum, as evidenced by the use case shown in the specification:
scoped_lock lock( m, want_to_lock? locking: non_locking );
I find this an unnecessarily verbose and contrived way to express
scoped_lock lock( m, want_to_lock );
which is the original intent.
So how do you feel about:
scoped_lock lk1(m, defer_lock); // not locked scoped_lock lk2(m, try_lock); // tries to lock
As others have shown, you can still decide at runtime with something like:
scoped_lock lk(want_to_lock ? scoped_lock(m) : scoped_lock(defer_lock));
I see. Not being able to decide at runtime was what shot down the type-based constructors for me (though I liked them lot) before; movable locks make it possible. However, if scoped_lock lock( m, want_to_lock? locking: non_locking ) is too verbose, this is even worse. To me the above syntax is ugly for something so common, and slightly inefficient. The free functions would be just as inefficient, I guess, but prettier.
or whatever. As Vladimir pointed out, these new constructors are very cheap and fast since they don't have to test anything to find out what they should do. The defer_lock constructor is especially fast. It just sets the internal mutex reference to m, and the locked_ to false (two, maybe 3 assembly instructions).
This is one of the things I really liked about the type-based constructor selection.
I disagree that
void * mutex() const;
is a better mutex() accessor. On the contrary, I'd argue that mutex() should return a non-const mutex_type. A const return doesn't make a lot of sense given that our mutexes have no const member functions; the private interface is a better protection than the const. On the other hand, a non-const return allows the client to use scoped_lock<> on user-defined mutex types (in implementing the user-defined condition::wait, for example).
I've gone with:
mutex_type* mutex() const;
I don't have very strong feelings on this one, except that the current boost design does not hide the mutex interface as we thought it should. You just get to the mutex interface via the lock instead of directly. So I'm not that anxious to make it difficult to access the mutex and operate on it directly. Sometimes there's good reason to do that.
What can you do to a mutex except lock it with a lock object?
I also like the proposed scoped_lock == write_lock unification.
<nod>
On Jul 19, 2004, at 2:05 PM, Eric Niebler wrote:
Movable locks open up all sorts of interesting interface possibilities.
<nods so agreeably that head almost falls off!> :-)
On Jul 19, 2004, at 6:28 PM, David Abrahams wrote:
"non_locking" is a terribly obscure name to associate with a lock. I really think "deferred" works well.
<nod> How's:
scoped_lock lk1(m, defer_lock); // not locked
Another possibility is to name the second parameter as an adjective describing the initial state of the lock instead of a verb: scoped_lock l(m, unlocked); Mike