Re: [boost] Re: Review: Boost.Threads

From: Ed Brey (edbrey_at_[hidden])
Date: 2001-09-10 09:13:41

From: <williamkempf_at_[hidden]>

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

For such a low-level class, I'm not sure how avoiding undefined behavior
by a run-time check is a good idea.  It seems similar to how it would be
if std::stack().pop() had a defined behavior.  One would be paying for
something that is never used.

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

In this case, it is exactly the cleanliness of the OO design that I find
the member functions break.  The reason is that the wait functions are
not primarily acting on the object, but rather on the current thread.
The item being waited for is just a parameter, much as is auto_ptr x in
the statement: y.reset(x).  In this case, the object being acted upon
doesn't have a variable name, since it is the current thread.  We could
make a global called this_thread, so one could have
this_thread.wait_for(something).  That reads very nicely, but so does
wait_for(something), and it avoids the hassle of the global variable
just for syntactic sugar.  Still, the strongest argument seems to be how
the interface becomes very consistent and easy to read, with code always
looking like wait_for(something), where something is a magic overload of
all things that could be waited for.

[Naming of function to wait for a thread to complete.]
> 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.

I'm not sure I agree with that partitioning of responsibility, but I'll
table my thoughts, since the consistency discussion above seems more
poingant and may render this issue mute.

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

As another's has mentioned, if they are indeed in details, they should
be only documented in an implementation document, which should be kept
quite separate from the main documentation.

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

At the time I wrote that I was tricked into believing there was a thread
namespace by one of the out-of-date examples.  I'm fine with not having
a thread namespace.

[Including Windows.h in header]
> 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.

Understandable.  Just out of curiosity, what kind of applications are
out there that are compiled on win32, use threads, and don't alreayd
include <windows.h> for something else.

> > In condition::notify_all, line 125, signals is assigned with no
> effect.
>
> I'm not sure I follow.

The line is:
        m_waiting += (signals = m_blocked);

signals is a local built-in variable that is never again referenced in
the function.

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

I'm going off of the rule "use vector by default".  It seems that either
you know that the size is small enough that O(N) is acceptable, or you
don't know that and you need a better guarantee, in which case you need
a set.  If O(N) is acceptable, it probably means that the container is
small enough for a vector.

> I thought I'd caught all of them.  Are you looking at the corrected
> snapshot or the original?

I was looking at the threadsubmission.zip snapshot put up at the
beginning of the review process.

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

I don't understand the distinction.  Can you point me to somewhere where
I may educate myself.

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

The compiler doesn't give a hoot.  It just makes the code a tad easier
to read (for me anyway).

> I've never agreed with the level 4 approach, but I realize many
> others do.  I'll look into this as well.

The rationale is that there are many level 4 warnings that are good.
For example, some of the thread code assigns a 64-bit number to a 32-bit
without an indication that this was not done by accident.  Also, there
are places where a signed number is compared to an unsigned number,
again without indication that the boundary conditions have been
considered.  These items are picked up under warning level 4, and can
lead to better code.