Boost logo

Boost :

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


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