Boost logo

Boost :

From: williamkempf_at_[hidden]
Date: 2001-09-07 08:24:56

--- In boost_at_y..., John Maddock <John_Maddock_at_c...> wrote:
> First of all, let me say that I like the thread lib, and I think
that it
> should be accepted into boost. I think that the docs are
excellent; more
> specific comments follow (based on :
> Semaphores:
> ~~~~~~~~~
> I think that the semaphore type should be in the lib, However I
have one
> query about the interface - you have included in the constructor an
> to set the maximum value of the semaphore - as far as I know this
is a
> Win32 specific feature. Neither POSIX nor BeOS semaphores support
> While it is true that you can emulate this behaviour (probably
using two
> native semaphores), generally speaking a semaphore should be the
> lightweight synchronisation primitive available, adding this
feature makes
> it impossible to implement boost::semaphore in terms of a single
> lightweight native primitive.

It's not truly a MS thing. There's a lot of documentation out there
that describes both binary semaphores (the classic) and counting
semaphores (what Boost.Threads has), and a lot of this was published
before Win32 existed. There are a lot of benefits to a counting
semaphore and native implementations should be doable on any platform
(though you're correct that I've emulated them for POSIX today).

> In Semaphore::up the assert:
> assert(ret || GetLastError() == ERROR_TOO_MANY_POSTS);
> is failing for the semaphore test in thread_test.cpp, it seems as
> GetLastError is returning ERROR_INVALID_PARAMETER rather than
> ERROR_TOO_MANY_POSTS on my Win98 box.

Hmmm... I get no failures on Win2k. I'll have to research.
> Mutex:
> ~~~~
> Ideally I would like each mutex type to be as efficient as
possible: on
> win32 that means using critical sections and not mutexes where
> (mutex and recursive_mutex). Last I heard, a win32 mutex is about
4x as
> slow as a critical section, and given that the simpler primitives
here are
> likely to be used in "contentious" situations, I would like them to
be as
> efficient as possible.

I've talked about this elsewhere.
> Should I be concerned that you cast a HMUTEX to an unsigned long?

Only mildly. This is precisely what MS themselves did in the C RTL
(look at the docu for _beginthreadex) to avoid inclusion of Windows
specific headers.

>Is this
> portable to all windows versions (32 and 64 bits, WinCE etc?). I
think I
> would be inclined to use a void* throughout, and remove the casts -
> way you'll get an error if its wrong, alternatively add a:
> BOOST_STATIC_ASSERT(sizeof(unsigned int) >= sizeof(HANDLE))
> to one of the cpp files.

I'm holding off on such changes to see precisely how MS themselves
address this in 64 bit.
> The Win32 mutex constructors look wrong: you have both an assert
and a
> (currently redundant) throw.

The assert allows for debugging while the throw insures an error is
raised in release. Can't really do with out either one. I will
however look to make sure there's not something wrong.
> Condition:
> ~~~~~~
> Is it correct that you can use a condition variable with any mutex
> I'm concerned that this places an unnecessary restriction on how
the mutex
> types are implemented, and in particular that it may prevent use of
> most efficient implementation for the simpler mutex types. I'm
> concerned about porting to new platforms/api's here.

It is correct, and it's not caused any problems so far. Can the
concern be made more real?
> What happens if the mutex is a recursive mutex, and has been locked
> multiple times - does it deadlock? If so that would be a good
reason not
> to use recursive mutexes with condition variables?

No, recursive mutexes have been specifically coded to work
correctly. :)
> If the condition variable supports timed waits, then does the mutex
> have be timed waitable? (I guess not?)

No, it doesn't.
> Once.cpp:
> ~~~~~~~
> You seem to be using swprintf without including the necessary header
> (wchar.h), you are also using a non-standard (MS specific) version
of that
> function, here's what C99 says about it:
> The swprintf function
> Synopsis
> 1 #include <wchar.h>
> int swprintf(wchar_t * restrict s,
> size_t n,
> const wchar_t * restrict format, ...);
> Note the extra size_t parameter - we have a macro for this BTW (or
> have when the new config gets merged in): BOOST_NO_STD_SWPRINTF.

This is used only on MS platforms so I don't think it's an issue.
However, it will be going away since UNICODE doesn't work on 9x.
> You also use the wide character version of CreateMutex here: but
that isn't
> supported on Win9x (and fails).

Pointed out, and will be changed.
> xtime.cpp:
> ~~~~~~
> I would prefer the section:
> #else
> return 0;
> #endif
> to be replaced by
> #else
> # error
> #endif
> It makes it easier to catch a mis-configured set of headers.

But won't be correct for the final "full" implementation, since not
all platforms will support all clock types.
> Headers:
> ~~~~~~
> Can we have a section that produces a #error if BOOST_HAS_THREADS is
> defined, but no supported threading API is configured?

With boost/thread/config.hpp this problem isn't possible.
BOOST_HAS_THREADS is conditionally defined dependent on those other
macros as well as detection of whether or not threading support
is "on" by the compiler. I'm not sure how to work this out with the
new config system. I'll have to get some help integrating this
portion if accepted.
> Config:
> ~~~~
> We need some test code for each of the macros (to slot into the
> split-config's regression tests, and configure script).
> We also need to figure out what the appropriate POSIX feature tests
are for
> some of these API's, and add them to the new config.

Some don't have feature tests, unfortunately. Getting the config
stuff correct is going to take some doing.
Bill Kempf

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