Re: [boost] Boost.Fiber mini-review September 4-13

Subject: Re: [boost] Boost.Fiber mini-review September 4-13
From: Agustín K-ballo Bergé (kaballo86_at_[hidden])
Date: 2015-09-04 15:05:51

On 9/4/2015 3:46 PM, Nat Goodspeed wrote:
> On Fri, Sep 4, 2015 at 2:24 PM, Agustín K-ballo Bergé
> <kaballo86_at_[hidden]> wrote:
>
>> I just had a quick look at the future implementation, to see if proper
>> chrono support (one of the reasons for my previous rejection) was
>> implemented. While `wait_for` depicts the standard signature, it does not
>> seem `wait_until` does.
>
> I want to make it as easy as possible for Oliver to act on people's
> feedback. Please suggest a specific change?

That would be:

"- Every API involving time point or duration should accept arbitrary
clock types, immediately converting to a canonical duration type for
internal use."

For further reference: 
http://talesofcpp.fusionfenix.com/post-15/rant-on-the-templated-nature-of-stdchrono

>> The shared state has a few functions that *must* be called while holding a
>> lock, like `mark_ready_and_notify_`. It'd be better to make that explicitly,
>> and preferably have the type system enforce it, as currently proving the
>> correctness of those functions require tracing all the callers.
>
> Is your suggestion that Fiber's use of its shared_state track more
> closely the way Boost.Thread uses its own shared_state?

I am not familiar with the way Boost.Thread uses its own shared_state. 
My concern is that functions like `mark_ready_and_notify_` have certain 
lock requirements that currently are implicit:

     void mark_ready_and_notify_() { // is there a race here??
         ready_ = true;
         waiters_.notify_all();
     }

To prove the correctness of those functions, one has to look up all 
calls to `mark_ready_and_notify_`, all calls to those calls, and so on.
To make the code readable, I would expect lock requirements to be stated 
explicitly, for instance:

     void mark_ready_and_notify_(std::unique_lock< mutex >& lk) {
         ready_ = true;
         waiters_.notify_all();
     }

Although holding a mutex while firing on the condition variable is 
considered bad practice, so this would be even better:

     void mark_ready_and_notify_(std::unique_lock< mutex >&& lk) {
         ready_ = true;
         lk.unlock();
         waiters_.notify_all();
     }

Regards,
-- 
Agustín K-ballo Bergé.-
http://talesofcpp.fusionfenix.com