Boost.Threads proposed change: lock constructors
Below I detail proposed changes to the Boost.Threads lock class constructors. Comments? Questions? Mike ----------------------------------------------------------------- The Boost.Threads lock concept constructors are currently defined as follows: Definitions ----------- L: lock type l: lock variable (type L) m: mutex variable (appropriate Mutex type) s: initial lock state (type bool, true=locked, false=unlocked) Lock Concept ------------ L l(m,s) //Blocking lock if s L l(m) //Blocking lock TryLock Concept --------------- L l(m,s) //Blocking lock if s L l(m) //Non-blocking lock TimedLock Concept ----------------- L l(m,s) //Blocking lock if s L l(m,t) //Blocking lock, failing if not obtained by time t These definitions have the following drawbacks: 1) That one of the TryLock constructors blocks while the other doesn't is neither obvious nor very consistent. 2) That the single-parameter constructor of Lock blocks while the single-parameter constructor of TryLock doesn't is neither obvious nor very consistent. 3) Part of the reason for the above inconsistencies is that it's not clear whether the constructors of TryLock should block or not. 4) The TimedLock constructors have no way to request a non-blocking lock. To address these problems, I would like to redefine the lock concept constructors as follows (changes indicated with "^"): Definitions ----------- L: lock type l: lock variable (type L) m: mutex variable (appropriate Mutex type) s: initial lock state (enum type {NO_LOCK=0, LOCK=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ b: block? (enum type blocking_state {NON_BLOCKING=0, BLOCKING=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Lock Concept ------------ L l(m,s) //Blocking lock if s L l(m) //Blocking lock TryLock Concept --------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,b) //Lock; blocking if b ^ ^^^^^^^^^^^^^ TimedLock Concept ----------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,t) //Blocking lock, failing if not obtained by time t Note: s and b are defined as enums rather than bools to prevent existing code from breaking silently: e.g., if s and b were bools, "TryLock l(m, false)", which currently means "construct a lock object but don't obtain a lock", would change with the new definitions to mean "construct a lock and make a non-blocking attempt to obtain a lock". Also, enums are more explicit than bools. Note: these changes will cause some existing code to break by producing compile errors; the compile errors can be fixed by appropriately adding or changing parameters being passed to the changed TryLock and TimedLock constructors. Mike
No comments? Perhaps my original message is too long or too abstract. Let me summarize. For the reasons mentioned in my original post (primarily fixing inconsistencies and unintuitive behavior), I propose changing: 1: explicit scoped_lock(Mutex& mx, bool initially_locked=true) 2: explicit scoped_try_lock(TryMutex& mx) 3: scoped_try_lock(TryMutex& mx, bool initially_locked) 4: scoped_timed_lock(TimedMutex& mx, bool initially_locked) to: 1: explicit scoped_lock(Mutex& mx, lock_state initial_state=LOCK) 2: scoped_try_lock(TryMutex& mx, blocking_mode blocking) 3: scoped_try_lock(TryMutex& mx, lock_state initial_state, blocking_mode blocking) 4: scoped_timed_lock(TimedMutex& mx, lock_state initial_state) where lock_state is defined something like this: namespace lock_state { typedef enum { unlocked=0, locked=1 } lock_state; } and blocking_mode is defined something like this: namespace blocking_mode { typedef enum { non_blocking=0, blocking=1 } blocking_mode; } Mike
Michael Glassford wrote:
To address these problems, I would like to redefine the lock concept constructors as follows (changes indicated with "^"):
Definitions ----------- L: lock type l: lock variable (type L) m: mutex variable (appropriate Mutex type) s: initial lock state (enum type {NO_LOCK=0, LOCK=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ b: block? (enum type blocking_state {NON_BLOCKING=0, BLOCKING=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Lock Concept ------------ L l(m,s) //Blocking lock if s L l(m) //Blocking lock
TryLock Concept --------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,b) //Lock; blocking if b ^ ^^^^^^^^^^^^^
TimedLock Concept ----------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,t) //Blocking lock, failing if not obtained by time t
I like this proposal, if for no other reason than the move to enums makes the arguments clearer. A couple of points: TryLock: What would be the semantics of l(m, NO_LOCK, b)? In other words, if you're not going to lock on construction, why specify a blocking policy? L l(m, b) and L l(m, s) seem to cover the two scenarios: either I don't want to lock, or I do and I need to specify the blocking policy. TimedLock: I'm not sure I follow the reasons why TimedMutex refines TryMutex. (This is a comment on the original design, not your specific changed, but bear with me.) I think a timeout of zero seconds achieves the same goal, even if TimedLock/TimedMutex don't implement the same operations. So I don't think that TimedLock needs a constructor that takes a blocking argument, and ultimately I think TimedLock/TimedMutex should refine Mutex/Lock, and not TryMutex/TryLock. Also, the current implementation of TimedLock doesn't support a single argument constructor, even though it is supposedly a refinement of TryLock, which does. I'd like to see TimedLock implemented in such a way that I could use it as if it were a simple Lock, which I currently cannot. In summary, this is what I'd like to see: Lock Concept --------------- L l(m) //Blocking lock L l(m,s) //Blocking lock if s TryLock Concept --------------- As Lock, *plus:* L l(m,b) //Lock; blocking if b TimedLock Concept ----------------- As Lock (not TryLock), *plus:* L l(m,t) //Blocking lock, failing if not obtained by time t //If t == 0, acts as TryLock. Comments and criticism welcome, Christopher
Christopher Currie wrote:
Michael Glassford wrote:
To address these problems, I would like to redefine the lock concept constructors as follows (changes indicated with "^"):
Definitions ----------- L: lock type l: lock variable (type L) m: mutex variable (appropriate Mutex type) s: initial lock state (enum type {NO_LOCK=0, LOCK=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ b: block? (enum type blocking_state {NON_BLOCKING=0, BLOCKING=1}) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Lock Concept ------------ L l(m,s) //Blocking lock if s L l(m) //Blocking lock
TryLock Concept --------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,b) //Lock; blocking if b ^ ^^^^^^^^^^^^^
TimedLock Concept ----------------- L l(m,s,b) //Lock if s; blocking if b ^ ^^^^^^^^^^^^^ L l(m,t) //Blocking lock, failing if not obtained by time t
I like this proposal, if for no other reason than the move to enums makes the arguments clearer. A couple of points:
Yes, I like LOCK and NO_LOCK better than true and false for the same reason.
TryLock: What would be the semantics of l(m, NO_LOCK, b)? In other words, if you're not going to lock on construction, why specify a blocking policy?
The only reason is so that if you write l(m, LOCK, ...) you could also specify the blocking parameter. An expansion of Vladimir Batov's of using structs rather than enums could help here: struct nolock_t {}; nolock_t nolock; struct lock_t {}; lock_t lock; class TryLock { TryLock(TryMutex m, no_lock_t s) {...} TryLock(TryMutex m, lock_t s, blocking_t b) {...} }
L l(m, b) and L l(m, s) seem to cover the two scenarios: either I don't want to lock, or I do and I need to specify the blocking policy.
TimedLock: I'm not sure I follow the reasons why TimedMutex refines TryMutex. (This is a comment on the original design, not your specific changed, but bear with me.) I think a timeout of zero seconds achieves the same goal, even if TimedLock/TimedMutex don't implement the same operations. So I don't think that TimedLock needs a constructor that takes a blocking argument, and ultimately I think TimedLock/TimedMutex should refine Mutex/Lock, and not TryMutex/TryLock.
Also, the current implementation of TimedLock doesn't support a single argument constructor, even though it is supposedly a refinement of TryLock, which does. I'd like to see TimedLock implemented in such a way that I could use it as if it were a simple Lock, which I currently cannot. In summary, this is what I'd like to see:
Lock Concept --------------- L l(m) //Blocking lock L l(m,s) //Blocking lock if s
TryLock Concept --------------- As Lock, *plus:* L l(m,b) //Lock; blocking if b
TimedLock Concept ----------------- As Lock (not TryLock), *plus:* L l(m,t) //Blocking lock, failing if not obtained by time t //If t == 0, acts as TryLock.
As I said in another post, I'll think about this some more. Thanks for taking the time to comment. Mike
Michael Glassford wrote:
Christopher Currie wrote:
TryLock: What would be the semantics of l(m, NO_LOCK, b)? In other words, if you're not going to lock on construction, why specify a blocking policy?
The only reason is so that if you write l(m, LOCK, ...) you could also specify the blocking parameter. An expansion of Vladimir Batov's of using structs rather than enums could help here:
struct nolock_t {}; nolock_t nolock;
struct lock_t {}; lock_t lock;
class TryLock { TryLock(TryMutex m, no_lock_t s) {...}
TryLock(TryMutex m, lock_t s, blocking_t b) {...} }
An interesting syntax, I can see how it does make explicit whether you are locking or not. Personally, I dislike having to type an extra argument when it's the only choice; IMO if you're specifying a blocking parameter, the locking is implied, and therefore superfluous. I was thinking something like (going back to enums for a moment): enum { unlocked = 0, locked } lock_init; enum { nonblocking = 0, blocking } blocking_action; class TryLock { public: TryLock( TryMutex m, lock_init s = locked ) { ... } TryLock( TryMutex m, blocking_action b ) { ... } }; Also, going back to the struct technique, do the struct instances need to be in an anonymous namespace to prevent ODR violations? Just trying to get my head around the concept. Christopher -- Christopher Currie <codemonkey@gmail.com>
Christopher Currie wrote:
Michael Glassford wrote:
Christopher Currie wrote:
TryLock: What would be the semantics of l(m, NO_LOCK, b)? In other words, if you're not going to lock on construction, why specify a blocking policy?
The only reason is so that if you write l(m, LOCK, ...) you could also specify the blocking parameter. An expansion of Vladimir Batov's of using structs rather than enums could help here:
struct nolock_t {}; nolock_t nolock;
struct lock_t {}; lock_t lock;
class TryLock { TryLock(TryMutex m, no_lock_t s) {...}
TryLock(TryMutex m, lock_t s, blocking_t b) {...} }
An interesting syntax, I can see how it does make explicit whether you are locking or not. Personally, I dislike having to type an extra argument when it's the only choice; IMO if you're specifying a blocking parameter, the locking is implied, and therefore superfluous.
That was an oversight on my part. Though in the case of read/write locks, you do need to specify both the lock type (read/write) and whether it is blocking or not.
I was thinking something like (going back to enums for a moment):
enum { unlocked = 0, locked } lock_init; enum { nonblocking = 0, blocking } blocking_action;
class TryLock { public: TryLock( TryMutex m, lock_init s = locked ) { ... }
TryLock( TryMutex m, blocking_action b ) { ... } };
Also, going back to the struct technique, do the struct instances need to be in an anonymous namespace to prevent ODR violations? Just trying to get my head around the concept.
Probably they should be in a separate namespace, though possibly not anonymous. Mike
Michael Glassford <glassfordm@hotmail.com> writes:
Also, going back to the struct technique, do the struct instances need to be in an anonymous namespace to prevent ODR violations? Just trying to get my head around the concept.
Probably they should be in a separate namespace, though possibly not anonymous.
Anonymous namespaces in header files *cause* ODR violations. Not sure how they can be used to prevent ODR violations. Peter, are you planning to get the bind placeholders out of the unnamed namespace? -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
David Abrahams wrote:
Michael Glassford <glassfordm@hotmail.com> writes:
Also, going back to the struct technique, do the struct instances need to be in an anonymous namespace to prevent ODR violations? Just trying to get my head around the concept.
Probably they should be in a separate namespace, though possibly not anonymous.
Anonymous namespaces in header files *cause* ODR violations. Not sure how they can be used to prevent ODR violations.
Peter, are you planning to get the bind placeholders out of the unnamed namespace?
I'm not sure how the bind placeholders can cause ODR violations; could you elaborate? Anyway, I guess we'll have to migrate to TR1 style placeholders one day, but this will break existing code.
"Peter Dimov" <pdimov@mmltd.net> writes:
David Abrahams wrote:
Michael Glassford <glassfordm@hotmail.com> writes:
Also, going back to the struct technique, do the struct instances need to be in an anonymous namespace to prevent ODR violations? Just trying to get my head around the concept.
Probably they should be in a separate namespace, though possibly not anonymous.
Anonymous namespaces in header files *cause* ODR violations. Not sure how they can be used to prevent ODR violations.
Peter, are you planning to get the bind placeholders out of the unnamed namespace?
I'm not sure how the bind placeholders can cause ODR violations; could you elaborate?
Simple: // f.hpp template <class S, class T> bool f(S const& s, T const& x) { return std::find_if( s.begin(), s.end() , boost::bind(std::less<T>(), _1, x)) != s.end(); } // t1.cpp #include "f.hpp" #include <vector> bool f1(std::vector<int> const& s, int x) { return f(s,x); } // t2.cpp #include "f.hpp" #include <vector> bool f2(std::vector<int> const& s, int x) { return f(s,x); } f<std::vector<int>,int> refers to a different object when it names "_1" in each translation unit.
Anyway, I guess we'll have to migrate to TR1 style placeholders one day, but this will break existing code.
I'm not sure it has to: namespace bind { extern arg<1> _1; } namespace { // if you like using bind::_1; } -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
David Abrahams wrote:
"Peter Dimov" <pdimov@mmltd.net> writes:
David Abrahams wrote:
I'm not sure how the bind placeholders can cause ODR violations; could you elaborate?
Simple:
// f.hpp template <class S, class T> bool f(S const& s, T const& x) { return std::find_if( s.begin(), s.end() , boost::bind(std::less<T>(), _1, x)) != s.end(); }
// t1.cpp #include "f.hpp" #include <vector> bool f1(std::vector<int> const& s, int x) { return f(s,x); }
// t2.cpp #include "f.hpp" #include <vector> bool f2(std::vector<int> const& s, int x) { return f(s,x); }
f<std::vector<int>,int> refers to a different object when it names "_1" in each translation unit.
Thanks.
Anyway, I guess we'll have to migrate to TR1 style placeholders one day, but this will break existing code.
I'm not sure it has to:
namespace bind { extern arg<1> _1; }
#1: extern implies .obj. Currently bind doesn't need a build/install phase.
namespace { // if you like using bind::_1; }
#2: using declarations of this form have a lot of potential. :-) I had in mind something like namespace boost { namespace placeholders // also sagt TR1 { inline boost::arg<1> _1() { return boost::arg<1>(); } } // placeholders } // boost #ifndef BOOST_BIND_NO_GLOBAL_PLACEHOLDERS using namespace boost::placeholders; #endif But not for 1.32, I think. First thing post-branch, maybe. ;-)
"Peter Dimov" <pdimov@mmltd.net> writes:
Anyway, I guess we'll have to migrate to TR1 style placeholders one day, but this will break existing code.
I'm not sure it has to:
namespace bind { extern arg<1> _1; }
#1: extern implies .obj. Currently bind doesn't need a build/install phase.
namespace { // if you like using bind::_1; }
#2: using declarations of this form have a lot of potential. :-)
I had in mind something like
namespace boost {
namespace placeholders // also sagt TR1 {
inline boost::arg<1> _1() { return boost::arg<1>(); }
} // placeholders
} // boost
That works.
#ifndef BOOST_BIND_NO_GLOBAL_PLACEHOLDERS
using namespace boost::placeholders;
#endif
The above still makes me very nervous, unless (possibly) BOOST_BIND_NO_GLOBAL_PLACEHOLDERS is the default.
But not for 1.32, I think. First thing post-branch, maybe. ;-)
Probably. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
participants (4)
-
Christopher Currie -
David Abrahams -
Michael Glassford -
Peter Dimov