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
> should be accepted into boost. I think that the docs are
> specific comments follow (based on threadcorrect1.zip) :
> I think that the semaphore type should be in the lib, However I
> 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
> Win32 specific feature. Neither POSIX nor BeOS semaphores support
> While it is true that you can emulate this behaviour (probably
> native semaphores), generally speaking a semaphore should be the
> lightweight synchronisation primitive available, adding this
> 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.
> Ideally I would like each mutex type to be as efficient as
> win32 that means using critical sections and not mutexes where
> (mutex and recursive_mutex). Last I heard, a win32 mutex is about
> slow as a critical section, and given that the simpler primitives
> likely to be used in "contentious" situations, I would like them to
> 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
> portable to all windows versions (32 and 64 bits, WinCE etc?). 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
> (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.
> Is it correct that you can use a condition variable with any mutex
> I'm concerned that this places an unnecessary restriction on how
> 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
> to use recursive mutexes with condition variables?
No, recursive mutexes have been specifically coded to work
> If the condition variable supports timed waits, then does the mutex
> have be timed waitable? (I guess not?)
No, it doesn't.
> You seem to be using swprintf without including the necessary header
> (wchar.h), you are also using a non-standard (MS specific) version
> function, here's what C99 says about it:
> 18.104.22.168 The swprintf function
> 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
> supported on Win9x (and fails).
Pointed out, and will be changed.
> I would prefer the section:
> return 0;
> to be replaced by
> # error
> 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.
> 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.
> 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
> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk