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

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