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
> and the docs. It looks like a good start, but I do have some
> 1) You should throw an exception if CreateSemaphore returns nil or
Other's have pointed out the lack of error handling. I did this
intentionally in the draft, but it's something that needs addressed
> 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
> semaphore is already at the max count is helpful? In general I
> this sort of thing since return codes are way too easy to ignore in
> also makes writing the Windows implementation dicier since you
> 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
> 5) To be consistent basic_lock should explicitly privately inherit
Ahh... typo. Thanks for catching that one.
> 6) It's better to provide an operator const void*() method instead
> operator bool(). (Unlike bool the const void* one won't implicitly
> 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
> only way to get this to happen is for the programmer to do
> like call unlock too many times. But this is a straight forward
> error, not a runtime error, so it seems like an assert is the
Simple errors can, and will occur:
// This code added in maintenance
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
> 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,
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
> 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-
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk