Re: [boost] Boost.Threads draft library submission

From: Jesse Jones (jejones_at_[hidden])
Date: 2001-03-16 01:45:15

I'm far from a threading expert, but I did take a quick look at the code
and the docs. It looks like a good start, but I do have some quibbles:

1) You should throw an exception if CreateSemaphore returns nil or
INVALID_HANDLE_VALUE.

2) CloseHandle can also fail, but you don't want to throw from a dtor so an
assert might be good.

3) Are there examples where having semaphore::up() return false if the
semaphore is already at the max count is helpful?  In general I don't like
this sort of thing since return codes are way too easy to ignore in C. It
also makes writing the Windows implementation dicier since you really want
to distinguish between failing due to max count and due to something like a
corrupted object (the ReleaseSemaphore documentation in MSDN doesn't
provide much guidance in how to do this).

4) semaphore::down() should throw if WaitForSingleObject returns WAIT_FAILED.

5) To be consistent basic_lock should explicitly privately inherit from
noncopyable.

6) It's better to provide an operator const void*() method instead of
operator bool(). (Unlike bool the const void* one won't implicitly convert
to umpteen different types).

7) I don't think the locker classes should themselves throw exceptions. The
only way to get this to happen is for the programmer to do something nasty
like call unlock too many times. But this is a straight forward programmer
error, not a runtime error, so it seems like an assert is the answer.

8) It's a little odd that you don't allow clients to call
basic_lock::lock() twice in a row since the mutex would catch any problems.
Admittedly this would be a weird thing to do, but the whole point of
allowing lock is to allow people to do things out of the ordinary, no?

9) I think the code would be clearer if member data names were decorated
somehow, eg fCount, mCount, _count, etc. :-)

  -- Jesse