Re: [boost] [fiber] ready for next review

Subject: Re: [boost] [fiber] ready for next review
From: Agustín K-ballo Bergé (kaballo86_at_[hidden])
Date: 2015-12-06 09:15:58

On 12/4/2015 11:48 AM, Agustín K-ballo Bergé wrote:
> On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
>> 2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86_at_[hidden]>:
>>
>>> Yes, in the documentation.
>>
>>
>> documentation might need some updates - my announcement was primarly
>> focused to the source code
>
> Fair enough, I was misguided by the "ready for next review" subject as
> well as the announcement that requests from the review have been
> addressed. This is obviously not the case, but we can still make a lot
> of progress based on source code adjustments only.

The destructor for `shared_state` contains the following code:

     if ( ready_) {
         reinterpret_cast< R const* >( storage_)->~R();
     }

As far as I can see, `ready_` will be set both when the shared state 
holds a value or an exception. This would mean a shared state holding an 
exception would run arbitrary code on random garbage during destruction 
(sidenote: how did this manage to eluded unit tests??).

The choice of a cast to `const` pointer for destruction is peculiar. Not 
wrong, just odd, since qualifiers stop being in effect when the 
destructor starts.

The choice of `reinterpret_cast` is questionable. It would be safer to 
replace it with the two `static_cast`s this use case is supposed to 
synthesize. It'd probable be good too to replace the "redundant" 
artificial array of one storage this `reinterpret_cast` is relying on.

...or maybe just use `optional<R>`?

Furthermore, the member function `reset` will simply set the `ready_` 
flag to `false`, leaking the result value if there is one.

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