[boost] Comments about N2320

From: Yuval Ronen (ronen_yuval_at_[hidden])
Date: 2007-10-28 14:53:46

Hi,
After reading most of N2320, I have a few comments:

* thread::id has operator== and operator!= declared twice. Once in 
namespace scope, and a second time inside class thread. Second time is 
with operator< and his friends.

* Thread cancellation is new, and 
disable_cancellation/restore_cancellation are even newer. They are new 
for C++ programmers, and maybe new for *all* programmers (I never heard 
of a language with them). I'm not sure if it's a good idea to 
standardize them before we get some real-life experience with thread 
cancellation.

* Time issues. To my eyes, it looks not pretty trying to get threading 
with time issues standardized before we have std::date_time. Making it a 
templated type not because we want genericity, but because we don't have 
the type we want yet, makes it look coerced. I think it's best to drop 
the time-related stuff, and add it properly together with date_time. 
Using the timed version of thread::join(), mutex::lock() and 
condition::wait() are very rare, and I think (hope) they can be 
implemented externally using native_handle().

* I see that you chose not go accept a mutex in condition's constructor. 
If that is so, why make the condition class templated, instead of only 
the wait() functions, as is in Boost.Thread?

* If C++0x is going to have lambda support (is it?), then maybe the 
not-accepting-predicate overloads of condition::wait are no longer 
needed, and can be removed? I think the major reason why people use that 
overload is because they are too lazy to write a functor. With core 
support for lambda, it's a breeze. This will solve the other problem 
(mentioned in some other thread in this ML) about absolute vs. relative 
times.

* Back then we had a discussion about unique_lock, which was never 
finished. I claim that there is no need for a state where mutex() != 0 
&& owns() == false, which means that defer_lock is not needed. There is 
one example in the document using defer_lock, which I think can be done 
in another way. That example goes like:

Record&
Record::operator=(const Record& r)
{
     if (this != &r)
     {
         std::unique_lock<std::mutex> this_lock(mut, std::defer_lock);
         std::unique_lock<std::mutex> that_lock(r.mut, std::defer_lock);
         std::lock(this_lock, that_lock);
         // Both source and destination are locked
         // Safe to assign
         // ...
     }
     return *this;
}

If we change the first 3 lines of the 'if' to:

std::lock(mut, r.mut);
std::unique_lock<std::mutex> this_lock(mut, std::accept_ownership);
std::unique_lock<std::mutex> that_lock(r.mut, std::accept_ownership);

then it works without defer_lock. This allows us to:
1. Make unique_lock's interface simpler.
2. Make sizeof(unique_lock) smaller - no need for bool owns.
3. Make unique_lock more similar to unique_ptr, which makes it more 
intuitive.
4. Make std::lock simpler in the way that it doesn't need to accept 
locks, only mutexes.

In general it seems you paid a lot of attention to being able to pass a 
lock instead of a mutex wherever possible. I think it's absolutely not 
necessary. It's an undue complication.

* Other than that - very nice :)

Yuval