Re: Boost.Threads draft library submission

From: williamkempf_at_[hidden]
Date: 2001-03-16 09:55:30

--- In boost_at_y..., Jesse Jones <jejones_at_a...> wrote:
> 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.

Other's have pointed out the lack of error handling.  I did this 
intentionally in the draft, but it's something that needs addressed 
before submission.

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

When used properly it's next to impossible for CloseHandle to fail, 
and the library insulates the user from misusing CloseHandle.  So I 
see no need to use an assert here.
 
> 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).

I can't give you specific examples, but this is the behavior one 
would expect.  Detecting the proper error condition in Win32 should 
be trivial.
 
> 5) To be consistent basic_lock should explicitly privately inherit 
from
> noncopyable.

Ahh... typo.  Thanks for catching that one.

> 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).

This argument has come up on here before, and I'm not 100% sure that 
a consensus opinion was reached on this.  Conversion to bool is more 
natural to my mind, and the implicit conversion "to umpteen different 
types" doesn't seem to dangerous to me... at least in this case.  
Changing this will be trivial, so I'll defer to the "experts" 
opinions on this, but I'm not sure that they've fully agreed with 
each other in the past.  Am I wrong?

> 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.

Simple errors can, and will occur:

boost::mutex::lock lock(mutex);
// This code added in maintenance
if (something)
{
    // ...
    lock.unlock();
}
// ...
lock.unlock();  // error!

Yes, these are programmer errors, but they are serious errors that 
result in exceptional conditions.  I think it's appropriate to throw 
here, and if you don't throw you'll have to instead return an error.  
An assert is not enough.
 
> 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?

It just helps to insure that it's less likely a programmer will 
misuse the lock by declaring it outside of block scope (like at the 
class level).  It doesn't fully enforce this, so it may be overkill.  
Given the reasoning for it do you (and others) still think it should 
not throw in this case?

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

Stylistic suggestion, typically not entertained by Boost 
submissions.  However, I'm interested in this topic and wouldn't mind 
hearing what others have to say on this, even if it's in private e-
mail.

Bill Kempf