I apologize in advance for the rather meek review below; I did not have the time to be more thorough.
- What is your evaluation of the design?
The review discussions provided many insightful arguments in favour or against various aspects of the design, and I have not formed a final opinion about most of these. I do believe that fixing E for result<T> to error_code is the right decision. I would prefer a design without an empty state, because we already have a vocabulary type for it. The choice of C++14 seems motivated solely by performance considerations. A slower C++11 version should be considered.
- What is your evaluation of the implementation?
I did not have enough time to review it.
- What is your evaluation of the documentation?
The introduction (Description) only provides a vague description of the
purpose of the library. For example, it does not mention that its types
are used to carry either a value or an error. Instead, it assumes that
the reader can derive this from prior familiarity with expected<T> (or
Rust.)
The first motivating example is too large (as indeed are the examples
in the tutorial.) Instead I suggest a "Tony table" that shows the
essense of the library (either value or error.) For example:
Before:
FILE *file = fopen("myfile", "r");
if (!file)
throw std::system_error(errno, std::system_category(),
"Error delivered out-of-band by errno");
fclose(file);
After:
result
- What is your evaluation of the potential usefulness of the library?
The idea of an either-value-or-error return type is very useful.
- Do you think the library should be accepted as a Boost library?
No, not in its current form. I have two reasons for this rejection. First, some of the design discussions (especially on narrow/wide contracts) are not settled yet, so it would be premature to vote for the design. Second, whilst the lastest offer from the author about a fully-fledged Boost.Outcome repository without a dependency on the non-reviewed library called "boost-lite" is a step in the right direction, it is unclear to me how that repository will end up. Having been surprised by the current submission and the author's opinions, I will defer any vote of acceptance until the a repository is submitted for review.
The library is weakly motivated -- the motivation boils down to a less expensive alternative to throwing exceptions. A stronger motivation would have been to mention use cases were errors cannot be propagated by throwing exceptions. Here are two examples:
Thanks for the review. You may be interested to know than an earlier edition of the tutorial made use of ASIO as an example, it attracted a lot of criticism for being too hard to understand. I also had an example using concurrency, I was told to ditch that as well. What remains is a very simple threadpool implementation in the tutorial. I am also mindful in all these reviews suggesting things for the documentation that the average end user is not a Boost library developer. A tutorial aimed at the likes of us is considered far too terse and dense for say the average Reddit /r/cpp developer. But thank you anyway for the review and suggestions. I'll see what I can do. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 06/03/2017 12:21 AM, Niall Douglas via Boost wrote:
You may be interested to know than an earlier edition of the tutorial made use of ASIO as an example, it attracted a lot of criticism for being too hard to understand. I also had an example using concurrency, I was told to ditch that as well. What remains is a very simple threadpool implementation in the tutorial.
Just to be clear, I was not arguing for elaborate examples with Boost.Asio or concurrency in the documentation. I was suggesting that those use cases (more or less the text I wrote) are used for the motivation of the library.
participants (2)
-
Bjorn Reese
-
Niall Douglas