AMDG Andrey Semashev wrote:
Right, thanks for finding this. I'll try to fix it. I guess, I'll have to create a global mutex object for the assignment operations and lock it prior to other two locks and assignment. I think that this works: locking_state_machine& operator= (locking_state_machine const& that) { if(boost::addressof(that) != this) { const bool lock_this_first = boost::addressof(m_Mutex) < boost::addressof(that.m_Mutex); scoped_lock that_lock(lock_this_first? m_Mutex : that.m_Mutex); scoped_lock this_lock(lock_this_first? that.m_Mutex : m_Mutex); base_type::operator= (static_cast< base_type const& >(that)); } return *this; }
In Christ, Steven Watanabe
Hello Steven, Tuesday, January 9, 2007, 11:29:04 PM, you wrote:
AMDG
Andrey Semashev wrote:
Right, thanks for finding this. I'll try to fix it. I guess, I'll have to create a global mutex object for the assignment operations and lock it prior to other two locks and assignment. I think that this works: locking_state_machine& operator= (locking_state_machine const& that) { if(boost::addressof(that) != this) { const bool lock_this_first = boost::addressof(m_Mutex) < boost::addressof(that.m_Mutex); scoped_lock that_lock(lock_this_first? m_Mutex : that.m_Mutex); scoped_lock this_lock(lock_this_first? that.m_Mutex : m_Mutex); base_type::operator= (static_cast< base_type const& >(that)); } return *this; }
Wow, that's really great! That'll surely address the problem with non-template operator. I'll think it over if something like this could be made for the template one. Thanks for the idea! -- Best regards, Andrey mailto:andysem@mail.ru
Steven Watanabe wrote:
AMDG
Andrey Semashev wrote:
Right, thanks for finding this. I'll try to fix it. I guess, I'll have to create a global mutex object for the assignment operations and lock it prior to other two locks and assignment. I think that this works: locking_state_machine& operator= (locking_state_machine const& that) { if(boost::addressof(that) != this) { const bool lock_this_first = boost::addressof(m_Mutex) < boost::addressof(that.m_Mutex); scoped_lock that_lock(lock_this_first? m_Mutex : that.m_Mutex); scoped_lock this_lock(lock_this_first? that.m_Mutex : m_Mutex); base_type::operator= (static_cast< base_type const& >(that)); } return *this; }
You should also consider not extending the critical sections beyond the minimum necessary, as in locking_state_machine& operator= (locking_state_machine const& that) { scoped_lock that_lock( that.m_Mutex ); base_type tmp( that ); that_lock.unlock(); scoped_lock this_lock( m_Mutex ); tmp.swap( *this ); this_lock.unlock(); return *this; }
Hello Peter, Wednesday, January 10, 2007, 12:02:17 AM, you wrote:
You should also consider not extending the critical sections beyond the minimum necessary, as in
locking_state_machine& operator= (locking_state_machine const& that) { scoped_lock that_lock( that.m_Mutex ); base_type tmp( that ); that_lock.unlock();
scoped_lock this_lock( m_Mutex ); tmp.swap( *this ); this_lock.unlock();
return *this; }
I thought about something like this and decided not to do it. The implementation has no support for "swap" and it would not be an easy and convenient for users thing to implement it. Besides, the assignment creates an additional copy of the machine which is at least not intuitive for users and may considerably increase performance cost. -- Best regards, Andrey mailto:andysem@mail.ru
Andrey Semashev wrote:
Hello Peter,
Wednesday, January 10, 2007, 12:02:17 AM, you wrote:
You should also consider not extending the critical sections beyond the minimum necessary, as in
locking_state_machine& operator= (locking_state_machine const& that) { scoped_lock that_lock( that.m_Mutex ); base_type tmp( that ); that_lock.unlock();
scoped_lock this_lock( m_Mutex ); tmp.swap( *this ); this_lock.unlock();
return *this; }
I thought about something like this and decided not to do it. The implementation has no support for "swap" and it would not be an easy and convenient for users thing to implement it. Besides, the assignment creates an additional copy of the machine which is at least not intuitive for users and may considerably increase performance cost.
Yes, after looking at your submission, I now realize that base_type is basically a tuple of states (this could be a good use case for fusion). As a side note, speaking of performance, if you want to position your library as faster than Statechart, you probably need to back that up with specific measurements. These can go in the Performance section.
Hello Peter, Wednesday, January 10, 2007, 10:58:32 PM, you wrote:
Andrey Semashev wrote:
Hello Peter,
Wednesday, January 10, 2007, 12:02:17 AM, you wrote:
You should also consider not extending the critical sections beyond the minimum necessary, as in
locking_state_machine& operator= (locking_state_machine const& that) { scoped_lock that_lock( that.m_Mutex ); base_type tmp( that ); that_lock.unlock();
scoped_lock this_lock( m_Mutex ); tmp.swap( *this ); this_lock.unlock();
return *this; }
I thought about something like this and decided not to do it. The implementation has no support for "swap" and it would not be an easy and convenient for users thing to implement it. Besides, the assignment creates an additional copy of the machine which is at least not intuitive for users and may considerably increase performance cost.
Yes, after looking at your submission, I now realize that base_type is basically a tuple of states (this could be a good use case for fusion). As a side note, speaking of performance, if you want to position your library as faster than Statechart, you probably need to back that up with specific measurements. These can go in the Performance section.
Ok, I've put that in my to-do list. -- Best regards, Andrey mailto:andysem@mail.ru
Hello Steven, Tuesday, January 9, 2007, 11:29:04 PM, you wrote:
AMDG
Andrey Semashev wrote:
Right, thanks for finding this. I'll try to fix it. I guess, I'll have to create a global mutex object for the assignment operations and lock it prior to other two locks and assignment. I think that this works: locking_state_machine& operator= (locking_state_machine const& that) { if(boost::addressof(that) != this) { const bool lock_this_first = boost::addressof(m_Mutex) < boost::addressof(that.m_Mutex); scoped_lock that_lock(lock_this_first? m_Mutex : that.m_Mutex); scoped_lock this_lock(lock_this_first? that.m_Mutex : m_Mutex); base_type::operator= (static_cast< base_type const& >(that)); } return *this; }
Fixed. I've attached the updated file. -- Best regards, Andrey mailto:andysem@mail.ru
participants (3)
-
Andrey Semashev -
Peter Dimov -
Steven Watanabe