Boost logo

Boost :

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

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

* 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

* 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::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
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 :)


Boost list run by bdawes at, gregod at, cpdaniel at, john at