On 12/3/2015 3:49 PM, Oliver Kowalke wrote:

2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé :

Where can we find the documentation that corresponds to this branch?

...

http://olk.github.io/libs/fiber/doc/html/

Is this the one that corresponds to the develop branch? I looked for three specific mistakes that I raised during the review, but nothing seems to have changed there... Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On 12/3/2015 5:50 PM, Oliver Kowalke wrote:

2015-12-03 21:45 GMT+01:00 Agustín K-ballo Bergé :

...

On 12/3/2015 3:49 PM, Oliver Kowalke wrote:

...

2015-12-03 13:26 GMT+01:00 Agustín K-ballo Bergé :

Where can we find the documentation that corresponds to this branch?

...

http://olk.github.io/libs/fiber/doc/html/

Is this the one that corresponds to the develop branch? I looked for three specific mistakes that I raised during the review, but nothing seems to have changed there...

in the documentation?

Yes, in the documentation. During the re-review process I raised three conceptual mistakes in the documentation, important things and not simply just typos. I see they are still there in the documentation that corresponds to the develop branch, where review feedback has supposedly been addressed. So I went looking at the code, and while I do see some problems I reported have been addressed, others have not. Could you please just let us know exactly which pieces of feedback did you address and which did you decide to ignore, so that we can make sure nothing has been unintentionally fell through the cracks again? Thank you. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On 12/4/2015 4:47 AM, Oliver Kowalke wrote:

2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé :

...

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.

...

During the re-review process I raised three conceptual mistakes in the documentation, important things and not simply just typos.

which one - I can't remember your concerns. maybe 'this_fiber::yield() (et al.) are valid from main()' issue?

Please go back to that thread where people put their time and effort at your disposal to provide feedback for your library. Take with you pen and paper, and write each piece of feedback down, even those you've already acted on. Then let us know which of them you have addressed, and how you have done so (accepted, discarded, reasons, etc). This will help us avoid wasting our time now looking for things that haven't yet been addressed, like documentation changes, and it will help you to get ready for the next round of review so that we don't waste everyone's time then. Note that I have provided extensive and detailed feedback during the first review, and a big chunk of it was ignored, lost, or dropped on the floor. I had to re-raise those concerns among the fresh ones during the re-review, and from the looks of it we are heading into the same situation yet again. I would very much like to avoid having to re-re-raise these concerns again... As for those three particular conceptual mistakes in the documentation I was talking about, those are: - Bad code in example leading to races in destruction, with a big bold incorrect explanation on how the code is "safe" when it isn't. - Inconsistent `condition_variable` / `condition_variable_any` definitions, where the interface of `condition_variable_any` is exposed twice. - Inconsistent and contradictory `condition_variable` / `condition_variable_any` requirements, which are even stricter than those of `std::condition_variable`.

...

I see they are still there in the documentation that corresponds to the develop branch, where review feedback has supposedly been addressed.

So I went looking at the code, and while I do see some problems I reported have been addressed, others have not. Could you please just let us know exactly which pieces of feedback did you address and which did you decide to ignore,

* addressed: - cross-thread fiber migration - suggestions related to scheduler structure - wait_until() accepts any supported time_point - mark_ready_and_notify_() accepts std::unique_lock<> - release mutex before condition_variable signals - async() does not return by std::move() - use of intrusive-lists - re-factoring of future<> and related classes - use of predicate-based condition_variable::wait() internally

The list of feedback you have received is considerably larger than this one, even without considering those concerns raised only during the first round of review which still need to be addressed.

* ignored: - C++11 equivalent for deferred calls (was impossible to get right for all use cases in boost.fiber)

Note that this was not an official part of my review, but a comment on the margin. It's a pitiful decision which renders the library useless to me for no good reason, since C++14 only brings about one and a half new features with no known workaround, neither of which is being exploited by the library. But since you bring it up, I would like to know which cases you found to be impossible to get right. Since this is supposed to be just a mechanical transformation, it would be good to know those corner cases where it falls short. Related to this was my also unofficial suggestion to reuse one of the many implementations of `index_sequence` within Boost implementation details when `std::index_sequence` is not available. I don't see it mentioned in these lists, and I can see no code changes were done in this area, so I would like to know whether this is missing from the ignored list or just missing from the to-do list. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On 12/4/2015 2:25 PM, Oliver Kowalke wrote:

2015-12-04 15:48 GMT+01:00 Agustín K-ballo Bergé :

Please go back to that thread where people put their time and effort at

...

your disposal to provide feedback for your library. Take with you pen and paper, and write each piece of feedback down, even those you've already acted on. Then let us know which of them you have addressed, and how you have done so (accepted, discarded, reasons, etc). This will help us avoid wasting our time now looking for things that haven't yet been addressed, like documentation changes, and it will help you to get ready for the next round of review so that we don't waste everyone's time then.

I'm sorry that I waste your time

Let's just focus on addressing all that feedback that has thus far been ignored.

...

Note that I have provided extensive and detailed feedback during the first review, and a big chunk of it was ignored, lost, or dropped on the floor.

I used the summary of the last review

Of all the messages I sent during the review, only one of them was directed at the review manager. I believe the goal of the review summary is to present us, reviewers and casual bystanders, with a general view of what went on during the review. I might be wrong at that, but I don't think the goal of the summary is to exempt you, the library author, of having to read all the discussion and feedback meant for you to improve the quality of your library. And I happen to think Nat did a great job with it. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé :

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é

...

:

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.

I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line.

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.

it is not an ordinary allocator - it's a 'stack allocator' placement new is used to allocate the control-structure on the fiber-stack (instead on using new -> heap)

2015-12-05 15:03 GMT+01:00 Agustín K-ballo Bergé :

I found an unusual pattern in the code, where memory is allocated via an allocator's `allocate` but afterwards `construct` is side-stepped and placement new is used instead. This breaks proper allocator support. Curiously `destroy` is correctly used down the line.

allocators: classes fixedsize_stack/segmented_stack etc. are not allocators you know by the C++standard(s). those classes are used to allocate stack space. The classes make use of fucntionallity not common for allocators you have had in mind - for instance adding a protected page at the end or using splitstack-functions like __splitstack_makecontext).

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()?

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

2015-12-05 15:08 GMT+01:00 Agustín K-ballo Bergé : The refactoring does not appear to have happened for `promise`. Is this

intentional, or just an oversight?

oversight

In the futures, `wait_for` and `wait_until` stand out as they have been defined to forward to base instead of simply using `using`. Is there a reason for that?

a false usage of directive usage together with templated member functions - fixed

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

...

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 value of `future_status` enumerators is shifted when compared to the corresponding `std::` definition. This could lead to subtle mistakes for people familiarized with the standard definition. I would suggest to simply follow the normative definition from the standard here. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On 12/5/2015 11:25 AM, Agustín K-ballo Bergé wrote:

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

...

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.

While tracing overload resolution inside the library, I noticed a regression in the way rvalues are handled (some of the intervening are not in Boost.Context).

Ugh, I should double check before sending... What I meant here is that some of the intervening pieces (the INVOKE related stuff) are now in Boost.Context detail namespace, so this issue affects both libraries.

When an argument whose type is deduced as `T&&` is used in a function type, like with `result_of`, it has to be specified as `T&&`. If the rvalue reference is omitted then cv-qualifiers will be dropped, and arrays and functions will turn into pointers.

Note this issue overlaps with already reported issues on incorrect result type computations, and invalid decay-copying.

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