Re: Boost.Threads draft library submission

From: williamkempf_at_[hidden]
Date: 2001-03-15 11:20:17

--- In boost_at_y..., Doug Gregor <gregod_at_r...> wrote:
> On Thursday 15 March 2001 09:30, you wrote:
> [snip]
> > > It builds fine on x86 Linux with a few minor changes:
> > > 	1) Use forward slashes instead of backslashes when including
> >
> > headers.
> >
> > > Windows will support #include <boost/header.hpp>, but not all 
Unix
> >
> > systems
> >
> > > (Linux included) support #include <boost\header.hpp>.
> >
> > Jeremy Siek already pointed this out to me and I attempted to 
change
> > all of them.  Which specific ones did I miss?
> 
> The from threads/boost subdirectory:
> atomic.hpp:21:#include <boost\config.hpp>
> atomic.hpp:27:#   include <boost\fast_mutex.hpp>
> condition.hpp:21:#include <boost\config.hpp>
> fast_mutex.hpp:21:#include <boost\config.hpp>
> fast_mutex.hpp:26:#include <boost\utility.hpp>
> fast_mutex.hpp:27:#include <boost\xlock.hpp>
> mutex.hpp:21:#include <boost\config.hpp>
> mutex.hpp:26:#include <boost\utility.hpp>
> mutex.hpp:27:#include <boost\xlock.hpp>
> recursive_mutex.hpp:21:#include <boost\config.hpp>
> recursive_mutex.hpp:26:#include <boost\utility.hpp>
> recursive_mutex.hpp:27:#include <boost\xlock.hpp>
> semaphore.hpp:21:#include <boost\config.hpp>
> semaphore.hpp:26:#include <boost\utility.hpp>
> thread.hpp:21:#include <boost\config.hpp>

Wow!  Hard to believe I missed this many of them!  Thanks for finding 
them.

> From the threads/src directory:
> condition.cpp:21:#   include <boost\semaphore.hpp>
> condition.cpp:22:#   include <boost\atomic.hpp>

These particular ones occurred because the code originally existed in 
a Windows specific implementation file.  At least I have an excuse 
for two of them ;).
 
> From the threads/example directory:
> monitor.cpp:3:#include <boost\condition.hpp>
> monitor.cpp:4:#include <boost\fast_mutex.hpp>
> monitor.cpp:5:#include <boost\mutex.hpp>
> monitor.cpp:6:#include <boost\recursive_mutex.hpp>

And I just didn't pay as much attention to the examples as I did the 
code...
 
> > > However, I have reproduced the deadlock problem with the example
> >
> > program.
> >
> > With what sort of frequency?  Do you have any way of telling 
where it
> > deadlocks for me?  Obviously I've got a race condition here and 
will
> > have to track it down and fix it.
> 
> Out of 100 runs, it deadlocked three times. I don't know of any 
easy way of 
> determining where the deadlock occurs, but I'll take a closer look 
at it 
> later.

Thanks, you've confirmed my own experience.  Definately a subtle race 
condition in the implementation.  I'll have to see if I can track 
down what it is.
 
> > > A compiler at GCC's highest warning level (-ansi -pedantic -
Wall)
> >
> > results in
> >
> > > some signed/unsigned comparison warnings in the pthread handling
> >
> > code.
> >
> > > Commonly the condition:
> > >
> > > if (milliseconds == -1)
> > >
> > > appears, where "milliseconds" is an unsigned int. Is this
> >
> > safe/intended?
> >
> > Intended.  I'm not 100% sure about safe.  If this is not portably
> > safe I'll need to change this to use numeric_limits<>::max.
> 
> I'm not sure how to decipher what the C++ standard says in this 
regard 
> (4.7/2), though it appears that it won't be safe if the 
architecture uses 
> something other than two's complement integer arithmetic. My 
personal opinion 
> is that it would be best to replace -1 with, e.g., 
> enum { FOREVER = std::numeric_limits<unsigned int>::max };

I'll do this.
 
> Here's the list of nitpicks the Comeau compiler found:
> - Can't use a ';' after '}' ending a namespace (happens everywhere)

My own misunderstanding of C++ syntax.  I'll fix this.

> - Add an extra line feed at the end of each line (happens 
everywhere)

This one I don't understand.  Is it a problem with the \r\n 
DOS/Windows line endings?

> - #include <ctime> to get std::clock_t. (condition.hpp)

Thanks.

> - Qualify clock_t as std::clock_t (condition.hpp, src/*.cpp)

Won't work with VC++, so I'll have to use the usual work arounds.  
Thanks.

> - Nested "impl" classes have to be declared friends of their 
enclosing 
> classes (semaphore.hpp, mutex.hpp, fast_mutex.hpp, 
recursive_mutex.hpp)

This one I don't buy.  The "nested class" does not use the "enclosing 
class" in any way while the "enclosing class" uses only public 
methods of the "nested class".  I see no need for friendship in 
either direction.

> - use 0 instead of NULL because many compilers pick up nonportable 
NULL 
> definitions (fast_mutex.cpp, mutex.cpp, recursive_mutex.cpp, 
semaphore.cpp)

Dang... this is one that I try not to do but bad habits let some slip 
through.  I'll search out and fix this one as well.

> - In the monitor example, a "void*" parameter is casted to "void (*)
()", 
> which is illegal. The attached monitor.cpp uses a simple dispatch 
object to 
> get around this limitation.

Thanks.  This code will be replaced with proper Boost alternatives in 
Phase 2, but I should make the "hack" legal code until then.

Again, thanks for the comments and pointers.  I'll be working on this 
stuff today and hopefully will have an update posted tonight or 
tomorrow incorporating things people have mentioned here.

Bill Kempf