Re: Review: Boost.Threads

From: williamkempf_at_[hidden]
Date: 2001-09-09 19:40:28

--- In boost_at_y..., "Ed Brey" <edbrey_at_y...> wrote:
> 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.

I was just (inconsistently with other areas I admit) avoiding 
undefined behavior.

> 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.

This debate doesn't have a clear winner in my mind.  Some argue for 
free functions and others argue for members, and the reasoning is 
sound for both sides.  I personally prefer members, even if they are 
static.  It's a clearer OO design, with functionality clearly 
encapsulated in the object's interface.
 
> 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 auto_ptr has been discussed.  I'll change this.  As for the docu, 
you're right.  I'll fix this as well.
 
> The thread subdirectory is inconsistent with the lack of a general
> thread subnamespace.

It's what was suggested by several folks, though.  This sort of issue 
is one I don't care one way or another about.
 
> 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.

I understand the reasoning here, I just don't care for more name 
debates :).  I'd do what ever was suggested, unfortunately I doubt 
there'd be consensus and Boost doesn't work on a democratic voting 
strategy.
 
> 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?

A lot, but I don't want to rehash most of it.  The biggest thing, to 
my mind, is that join() actually does more than just wait, it's also 
responsible for cleaning resources up.
 
> 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 docu should indicate they are in a detail namespace.  I can't 
verify right now.  As for the name, I borrowed the convention from 
the STL implementation I use (Plaugers).  I expect that this header 
will be more firmly moved into detail, however, so none of this 
should remain an issue.
 
> 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.

Boost.Threads currently isn't in a subnamespace, and I'm not sure it 
should be.  Beman doesn't think so, so I'm disinclined to do so.
 
> 
> 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.)

It's more a problem with macro clashes caused by Windows.h and 
portable code.  I'm not likely to budge on this, since inlining makes 
little difference in performance here.
 
> In condition::notify_all, line 125, signals is assigned with no 
effect.

I'm not sure I follow.
 
> 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.

I'm not sure that you can be sure that it will tend to be small.  I'm 
not sure this is an easy one to categorize for efficiency.
 
> Documentation:
> 
> boost:: in boost::noncopyable is superfluous.

I thought about this, but for someone unfamiliar with all of Boost 
it's hardly superflous.  It helps to understand the documentation.
 
> Private sometimes precedes public in examples.  HMO, they should be
> reversed.

I'd agree.  I'm surprised this is the case in some examples.  I'll 
have to look for it and correct.
 
> 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.

OK.
 
> 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.

I'll look into it.
 
> 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.

I think the concepts are more important than the reference of the 
actual types.  What is needed that will help with what you're saying 
here is a high level tutorial.  I expect to add one at some point.
 
> Examples that use thread::create and join_all are out of date.

I thought I'd caught all of them.  Are you looking at the corrected 
snapshot or the original?
 
> 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.

I'll look at this.  Beman suggested that Jeremy had a script that 
would pull out examples from HTML and compile them.  I need something 
like this to insure the examples work.
 
> 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.

Hmm... I need to look at the docu to comment on this one properly and 
I don't have the time right now.  I'll look at this.
 
> 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.

This particular naming I can agree with.  I'll change it.
 
> scoped_lock::lock: Effect of unchecked locking strategy is not 
listed in
> the table.

I'll look into this.
 
> condition::wait: The asterisk in *this->notify_one() et al. is 
illegal.

Yes, it would be.  I'll fix.
 
> 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.

There's a distinct difference between the "locked" state of the 
scoped_lock and the "locked" state of the mutex.  The policies refer 
to the locked state of the mutex.

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

Not sure that it matters.  I'll look into changing it.

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

Hmmm... I'll look at that.
 
> 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.

I've never agreed with the level 4 approach, but I realize many 
others do.  I'll look into this as well.
 
> 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.)

Yeah, someone else pointed out this should be a map.  I'll change 
this.
 
> 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.

The difference is the STLport.  I use the stock Dinkumware library.  
I compile and test with out these problems.  Not sure why the STL 
would make a difference here, but I'll investigate.  There appears to 
be other bugs reported in this.

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

Will do.  Thanks.
 
> Existence of <hr> is inconsistent between overloads of 
condition::wait
> versus condition::timed_wait

I'll fix that.
 
> thread::thread: Postcondition should be Precondition.

I'll look at it.
 
> thread example: iostram should be iostream.

Thanks.
 
Bill Kempf