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-05 11:29:14

On 12/5/2015 12:37 PM, Agustín K-ballo Bergé wrote:
> On 12/5/2015 12:05 PM, Oliver Kowalke wrote:
>> 2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé <kaballo86_at_[hidden]>:
>>> Furthermore, from a cursory look it seems C++11 allocators are not
>>> supported. The code assumes a C++03 allocator interface. If it is
>>> intentional then this requirement should be documented.
>>>
>>
>> In class promise and packaged_task probably simply copy&paste the code
>> from
>> the StackAllocator snippets.
>> What's wrong with allocator::allocate() + placement new instead of
>> allocator::allocate() + allocator::construct()?
>
> An allocator might do other stuff in `construct` besides placement-new
> construction, which is what the default `std::allocator_traits` does. An
> allocator might, for instance, choose to trace all calls and log them,
> or it might choose to defer construction to some other allocator.
> Allocators like `scoped_allocator_adaptor` inject additional arguments
> and/or select different constructors to call (although I do not think
> this applies here). An allocator might even reject to construct certain
> types or use certain types of arguments, which they do so by failing to
> instantiate `construct` (17.6.3.5 [allocator.requirements]/9).
>
> Furthermore, keep in mind that `allocator::allocate` does not
> necessarily return a **raw** pointer.
>
> Using `std::allocator_traits` here would, besides giving some support
> for C++11 allocators, also pick the right placement-new to call in the
> case of nasty overloads (current code correctly qualifies `::new` but
> fails to secure a `void*` argument). Also, use of the replaceable
> standard library placement new requires `#include <new>`.

Some more notes on allocator usage:

When constructing an object on allocated storage, there's a chance an 
exception is thrown. In that case, the allocated storage should not be 
leaked. This is at least a concern for `packaged_task`, that 
decay-copies user-defined types during construction.

When destroying an object which might (or in this case will) have an 
embedded allocator, the allocator has to be copied out first. This is to 
avoid accessing a destroyed object, as it could happen if an allocator 
`destroy` member function decides to run code after performing the 
destruction (again, it could for instance decide to log the operation to 
some file handle it keeps as a member).

Back to `packaged_task`, it's member function `reset` is not resetting 
the `packaged_task` but it resets the shared state instead. This is not 
how the standard defines this operation, and it is a particularly 
dangerous semantic to have. Once a shared state is observed as ready it 
cannot go back to not ready, and it's result cannot be overwritten. 
Resetting a `packaged_task` requires allocating a new shared state and 
moving the type-erased target callable from the old one to it. That 
said, if the shared state is known to be non-observable (the 
`packaged_task` itself holds the last reference to it), then reusing the 
shared state is a viable optimization.

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