Date: 2001-09-01 15:17:15
--- In boost_at_y..., "Peter Dimov" <pdimov_at_m...> wrote:
> From: "William Kempf" <williamkempf_at_h...>
> > From: "Peter Dimov" <pdimov_at_m...>
> > >General comments:
> > >
> > >I don't consider phase 2 complete. Adding thread management
> > >may result in significant changes in the underlying
> > >even affect the design.
> > Effecting the implementation should be irrelevant, no? I don't
> > interface will be changed because of the addition of thread
> > functionaltiy (in fact, it should not).
> I'm wearing my 'pedantic reviewer' hat now (it's as if I see the
> for the first time.) Reviewing an incomplete design is dangerous.
If I were
> to take the submission at face value, my comments would be:
> "The boost::thread default constructor should be removed. There are
> ways to invoke undefined behavior using a default-constructed
object and too
> few legal operations; the only operation that makes sense that I
see is t ==
> thread(), which could have easily been provided in the (more
> t.is_current() form."
I'm not sure that would be more efficient, but otherwise I understand
what you're saying. There's no doubt that we need further
functionality for boost::thread and with out this added functionality
your points stand here. The additional functionality is going to
entail a lot of design work, though, and there's been pressure to get
at least a minimal library placed into Boost. I may not have made
the right decision here, but that is the decision that was made.
> > >The reference for many classes indicates that they derive from
> > >boost::noncopyable. This implies, AFAICS, that a C-style cast to
> > >boost::noncopyable* works. If retained, all such derivations
> > >'exposition-only' comment.
> > They indicate private derivation, so the cast isn't valid. So
> > the comment is needed, but I'm happy to add it.
> The C-style cast is, unfortunately, valid, even when the base class
> inaccessible. (5.4/7)
Hmmm... ugly. Can't say as I understand the reasoning for this. In
any event, the whole point of boost::noncopyable is to be used in
private derivation in this manner, so I wouldn't expect much
confusion with things as they are now. I will, however, add the
> > >Mutex:
> > >
> > >The reference section refers to the introduction for explanation
> > >scoped_lock nested type. I think that the reference should be
> > >self-contained.
> > Sorry, I don't follow what you mean here.
> This is probably a nit; I usually consider the Introduction section
> 'non-normative' (explains but does not strictly define) and the
> section 'normative' (strict definitions, no human-readable
I'll see if I can do something about this one.
> > There are numerous methods that can be employed to prevent
> > think this issue is really beyond the scope of the reference
> > for mutex. I can provide a FAQ on this one, which seems more
> Yes, it is outside the mutex scope. I'm mentioning it here for a
> better place. I think that the library should provide support for
> locking for two or more "mutex models," since this functionality is
> for deadlock prevention.
I mostly agree. The concept you've given won't always work (you
can't always acquire all the mutexes at once) and there are other
methods for avoiding deadlock (including the try_lock concept that is
included). Eventually we will need a concept such as this, it's just
a higher level concept than what's needed for this submission. As
long as the FAQ warns of the dangers and how to avoid it (only
newbies will not already know about this) then it's trivial enough to
avoid this problem with a pattern instead of a concrete concept model.
> > Yes, locking would be a cancellation point. However, two
> > locking are not needed. Cancellation concepts must include the
> > set the "cancellability state" of the thread, which allows you to
> > mutex with out the fear of the thread being cancelled.
> True, but tweaking the cancelability state is less elegant. In the
> case, you need to query the previous state, set it to disabled,
> mutex, restore the previous state. Which leads to problems if
> has altered the cancelability state between the save and the
Actually, changing the state is *more* elegant. During a destructor
you'll want all cancellation points ignored, not just a call to lock
a mutex. If a non-cancelling variant is needed for every
cancellation point then the library becomes more difficult to
understand as you have numerous seemingly identical methods. The
problems you refer to (restoring the previous state even in the face
of exceptions) are better solved through a RAII concept for disabling
> Of course this is not a showstopper for the current submission,
> something to keep in mind.
I've been thinking a lot about cancellation. I think I've got a
fairly good idea how to design this.
> > >I think that adopting standard POSIX terminology -
> > >of notify_one/notify_all - deserves consideration.
> > It did. In fact, it generated some debate on this list. I was
> > signal/broadcast in the beginning, but others convinced me the
> > ambiguous for those not "in the know". I don't care what names
> > frankly. I'm tired of name debates :). What ever is decided in
> > I'll go with.
> Name debates are fun. I'm not a big fan of join/signal/broadcast
> they have a clear advantage over the alternatives -- you can explain
> objectively why they were chosen.
With join you can, I'm not so sure about signal/broadcast. These
aren't "universally" accepted names for the operations like join is.
I can objectively explain why notify_one and notify_all were chosen,
even if I'd be just as happy (maybe happier) with signal/broadcast.
> > >The precondition '*this != thread()' is a specific instance of a
> > >problem. I don't really have a problem with it -- except that it
> > >well-defined behavior (deadlock) into undefined behavior (is
> > >it?), but I prefer something along the lines of 'The
> > >allowed but not required to detect a deadlock when this method
> > >When the implementation does detect a deadlock, the behavior is
> > implementation defined.'
> > Unfortunately, there is no "well defined behavior" here. If an
> > implementation detects this error and prevents it from happening
> > result is not deadlock. Explicitly stating that an
> > detect it and do something (implementation defined) is still the
> > undefined behavior. I don't mind adding this verbiage, I'm just
> > will help any.
> There is a difference between 'undefined behavior'
> behavior' (1.3.13) and 'implementation defined behavior.' (1.3.5)
> 'Undefined' means no guarantees at all.
> 'Unspecified' means that the implementation must choose a behavior,
> not required to document the choice.
> 'Implementation defined' means that the implementation must choose a
> behavior and document the choice.
> So 'implementation defined' is more strict than 'undefined.'
a) For a portable use you have to treat all three types the same.
b) I'm uncertain as to whether POSIX or Win32 gaurantee any behavior
However, for the standard you are quite right about all of this, so
I'll add the verbiage.
> To return to our 'join' example, if you remove the *this != thread()
> precondition, the behavior of a thread joining itself is well
> thread blocks forever. This may be preferable to 'undefined
> on our particular implementation may mean rebooting the machine
> flushing the disk cache, for instance.)
I don't gaurantee that the thread will wait for ever, and your
suggestion to make it "implementation defined" wouldn't gaurantee
this either. I agree that the verbiage should be added to strengthen
the contract, but I do not agree that the result is "deadlock".
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk