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

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é :

...

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