Boost logo

Boost :

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` ( [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.


Agustín K-ballo Bergé.-

Boost list run by bdawes at, gregod at, cpdaniel at, john at