Boost logo

Boost :

From: Ed Brey (edbrey_at_[hidden])
Date: 2001-09-08 16:20:02


Following is my review of Boost.Threads. My role as reviewer is
independent of my role as review manager. All comments expressed herein
are mine only and do not reflect a summary of all the reviews (which I
haven't read yet :-).

Overall, the design and documentation are excellent. I specially like
the well-written introductions to each page of the documentation, plus
the timely danger indications in the reference material. The interface
is sound, with many good design decisions, but could be improved by a
few tweaks. I have read most of the documentation and some of the code,
and I have compiled and run the test.

Based on my review (only), I believe that Boost.Threads should be
accepted into Boost. However, its acceptance should be conditioned on
fixing compilation errors, getting the test to run successfully, and
eliminating meaningful warnings. Additionally acceptance should be
conditioned on a having a coordinated interface to functions that affect
current thread, including documented rationale (details of current
concerns and suggestions are in the body of the review).

Interface design:

semaphore: When would up() ever be called such that it would raise count
above max? I would have expected count <= max be a precondition to
calling up(). There's probably a good reason that I'm not think of. It
might be good to document what it is.

Since condition::wait and thread::join affect the current thread rather
than the object that are part of, I think the interface would be clearer
if they were free functions that took the item to wait for as a
parameter. True, the user would need to qualify the name, i.e.
boost::wait(cond), which wouldn't otherwise be necessary; however, I
think the advantage of avoiding a form that makes it look like the
thread or condition is doing the waiting (or joining) something that is
not specified in the syntax is of greater value. Moreover, if waiting
on a condition, thread, and thread group were named consistently, e.g.
all using a function called wait, with a single using declaration the
user could have the nice syntaxes of: wait(my_thread),
wait(my_thread_group), or wait(my_condition). Likewise, sleep and yield
can be pulled out into free functions for convenience, since there is no
worry I can see of a name clash. Given these changes, wait, sleep, and
yield would all be convenient and consistent in syntax.

add_thread should take an auto_ptr. As it is, the client has to take
special precaution to avoid a leak when push_back fails within
add_thread. Also, the fact that add_thread may throw should be
documented.

The thread subdirectory is inconsistent with the lack of a general
thread subnamespace.

Naming:

Semaphore::up and down are adjectives doing a verb's work. More to
spirit of C++ would be the
use of operator++, operator--, and operator+= where no other parameters
are needed, and increment and decrement otherwise.

join: When a fresh user, not yet having picked up any jargon, asks "Why
did you name this function such-and-such?" the answer should be "because
that's what is does," and the fresh user, having read a simple function
description, should be able to say "Yes, you're right. I see that is
what it does." To this end, wait is clear, plus has the advantage of
terminology consistent with condition::wait. Are there any
disadvantages that I'm forgetting from the naming thread a while back?

xlock.hpp: The header name doesn't mean anything to me. I would have
thought just "lock.hpp". Complicating this issue is that the
documentation for scoped_lock et al. neglects to mention that they are
tucked within a detail namespace. They should either be true details
and not shown with any header or namespace (only accessible via the
mutex classes), or they should become first-class citizens of
Boost.Threads.

The subnamespace for Boost.Threads is thread, whereas for Boost.Tuples,
it is tuples. The singular vs. plural works against the cohesiveness of
Boost and lessens chances for standardization. I like the singular,
although that is difficult without adopting a changing the naming
convention to allow upper case. I don't have the answer to this issue,
but it is one that Boost needs to address.

Implementation:

Inlining should be used in some cases to increase efficiency. For
example, with functions that just call another function (with few
parameters), such as semaphore::down(). I realize that this may mean
including <windows.h> in the header, but this isn't a big problem. How
many users won't have it already? Plus with precompiled headers, the
time difference is negligible. (I realize that PCHs can worsen the
compiler sometimes, but it is fine to just turn them off for the
translation units that trigger the bugs.)

In condition::notify_all, line 125, signals is assigned with no effect.

thread_group::m_threads: Why not a vector? You are already paying O(N)
to do the linear search on removal. Since size() will generally be
small, a vector will tend to be more efficient.

Documentation:

boost:: in boost::noncopyable is superfluous.

Private sometimes precedes public in examples. HMO, they should be
reversed.

Documentation of constructions where you don't have anything to say
should be removed. For example, documentation of condition's
constructor and destructor should be removed, both in the members
section and in the synopsis. This way, whether condition actually has a
(manually created) constructor or destructor can be left as an
implementation detail and changed at any time.

semaphore: Plain English descriptions of up and down are hard to find.
They are buried in the introduction paragraph, but the reference section
only provides a "just read the source code" description.

There is room for improvement in organizing the documentation to steer
the end user right away towards the documentation that he is most likely
to find helpful, bearing in mind that he is likely to take a top-down
approach in getting a handle on the library. Unfortunately, I don't
have any specific suggestions in this regard; only that perhaps the
thread creation and mutex classes should tend toward the top of the
opening page.

Examples that use thread::create and join_all are out of date.

Mutex: I'm probably missing something obvious, but it doesn't look like
the example will work. The count in global_data is initialized to 0,
and so each of the 4 calls to down() will see that it is 0 and block
indefinitely.

scoped_lock: Rather than including the unreferenced abbreviation mx, it
would better to just omit the token altogether, since it adds nothing
and actually detracts by providing the reader with one extra item to
process.

scoped_lock: lock_it is a independent clause doing an adjective's work.
initially_locked would be more suitable for a boolean. I realize each
of these points about naming are by themselves nits; however, when taken
as a whole, there is considerable value in using parts of speech
consistently. The part of speech subtly reinforces the meaning of the
name.

scoped_lock::lock: Effect of unchecked locking strategy is not listed in
the table.

condition::wait: The asterisk in *this->notify_one() et al. is illegal.

The enumeration of the various locking strategies and scheduling
policies do an excellent job of putting the nature of boost unspecified
policies in a clear light. The only exception is that the strategy on
unlocking is ambiguous. Only the checked strategy mentions this.
Recursive, unchecked, and unspecified are silent. I would have expected
at least the last two, if not all three remaining to not check, i.e.
make m_locked == true a precondition. Currently, it is not a
precondition, as the code for scoped_lock::unlock always checks whether
the lock is locked, even though the policy is "unchecked". This seems
inconsistent.

Coding details:

scope_lock: The friendship declaration exists in the public section.
Wouldn't it be more appropriate in the private section?

test_thread.cpp: There are many unreferenced dummy parameters, which
trigger warnings. The corresponding variables should be removed.

Compiling the test and implementation code generates many warnings under
VC6. The code should be improved so that it compiles without warning
under level 4, given that warnings that are not helpful are turned off,
e.g.: 4097 4127 4284 4290 4554 4505 4511 4512 4514 4706 4710 4786 4800
4355 4250 4291 4251 4275.

tss.cpp has an error in line 128. An assignment is attempted to the
constant value dereferenced from the set iterator. A map may be a
better choice here, or perhaps using a structure with a mutable member
instead of pair. (The frequent usage of pair makes this code hard to
read anyway.)

Given a quick-fix const_cast for line 128, the test compiles. Running
the test, however, generates many assertions on line 78 of tss. Let me
know if this is a new problem to you and you would like more info. My
platform is VC6SP5 STLport 4.5b8, December 1999 Platform SDK.

Typos:

~timed_mutex Dangers: Extra period. Same in the condition introduction.
It probably wouldn't hurt to grep everything for "..</p>".

Existence of <hr> is inconsistent between overloads of condition::wait
versus condition::timed_wait

thread::thread: Postcondition should be Precondition.

thread example: iostram should be iostream.

Again, good job! The practicality of addressing almost exclusively the
bad points can be misleading. The good far outweigh them.

- Ed


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk