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<FILE *> file = fopen_wrapper("myfile", "r"); if (!file) throw std::system_error(file.error(), "Error delivered in-band by file.error()"); fclose(file.value()); This may not look overly convincing, but it does demonstrate the either value or error aspect in a few lines. 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: One motivating use case where exceptions cannot be throw are callbacks, such as Boost.Asio asynchronous handlers. Here the return value is not returned by the asynchronous operation (the initiating function), but will be passed to the provided callback once the operation completes. In other words, the following synchronous operation return_type operation(arg_type, error_code&); is turned into this asynchronous operation: void async_operation(arg_type, void (*)(return_type, error_code)); Exceptions cannot be thrown into the functions (here the callback), so errors have to be passed as arguments (whether error_code or exception_ptr.) The above works if the return_type has a reasonable "unengaged" value that can be passed as the first argument in case of an error. While this is true for Boost.Asio, where return_type is a std::size_t, it is not necessarily true in general. A much better solution is to pass an outcome<T>, result<T>, or expected<T>: void async_operation(arg_type, void(*)(result<result_type>)); Another motivating use case is passing results between threads. For instance, in the Actor model of concurrency information is passed between actors/threads via a message queue. Some of these messages are replies to requests that may or may not have failed. A queue containing one of the Outcome types is well-suited for this kind of interaction. Apart from the above-mentioned shortcomings, I found the remainder of the documentation quite readable. I did not find the inheritance diagrams in the reference documentation useful though.
- 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