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