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-04 09:48:42

On 12/4/2015 4:47 AM, Oliver Kowalke wrote:
> 2015-12-03 22:21 GMT+01:00 Agustín K-ballo Bergé <kaballo86_at_[hidden]>:
>
>> 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