[boost] [outcome] Andrzej's review

Hi Everyone, This is my review of Boost.Outcome library. First, I want to thank Niall for taking the effort to write the library, promoting it, sharing with the community, and submitting for Boost review. I think that the library has already proven useful regardless of this review's outcome (or should I say 'result'). I recommend to REJECT the library at this point, and encourage the author to re-submit it after the list of what I consider issues -- described below -- has been addressed. Why reject and not conditionally accept? It is possible to say that I accept the library if this "delta" is applied to the current submission's state. Niall has indicated a couple of times that he is willing to apply even big changes, if this is what reviewers wish. But the amount of change that would be required, plus the uncertainty what the "delta" actually is, plus a general confusion as to what library is for, as reported by others (the effect of the current shape of documentation), in total make the conditional acceptance too fragile. What are we reviewing? It would be fair to assume that we are reviewing the master branch, SHA 4851926c1c601c0e9391b2d0d07fb17f634b3ecb, but there is also a "delta" described by Niall in high-level in one of the messages: http://boost.2283326.n4.nabble.com/outcome-High-level-summar y-of-review-feedback-accepted-so-far-td4694767.html. So maybe the master + that delta. It describes that we will have like 10 types, like outcome_e, checked_outcome_e, etc.. Now in another thread I hear there will only be 5 types: optional_outcome and outcome. This makes me believe that the compromise solutions and promises are being made in a hurry, and this will likely result in (1) suboptimal design and (2) misunderstandings as to what we are actually agreeing to. Am I knowledgeable about the problem domain? If the domain is "reporting function failures in predictable-latency environments", then no, I have no experience with this domain. I also have no experience with `expected`, although having worked on formal specification of `std::optional` I am familiar with a number of issues that `expected` also needs to solve. Also, a number of controversies revolves around git sub-modules, dual licensing and Boost distribution process. I do not feel competent in this area either. How much effort did I put into the evaluation? I have read a number of versions of the documentation. Had a long email exchange with Niall in order to understand what the library does. I had a glimpse at the source code to figure out the pieces that were missing from documentation. I have tried running my own test examples on GCC 6.3.0 with MinGW w64 5.0.0 (doesn't work) and GCC 6.3.1 on Fedora (works fine). Is the library useful? Yes. Apparently it solves an important problem in predictable-latency environments. This is confirmed by the user base the library already has managed to acquire. It provides an implementation of the to-be standardized `exceptional`, which allows for experimentation. I believe Boost would benefit from including this library (after the issues have been resolved). I am confident Niall has knowledge about problem domain. Documentation. It has two serious deficiencies. It lacks an introductory part that would clearly and shortly describe what problem the library is solving an how, how can I use it and when should I use it. It is also missing a reference with documented semantics. One cannot tell what outocme's default constructor does, or what function error() does when you have an exception_ptr inside. I believe Niall when he says his users do not need this documentation: they immediately grasp what and how it is doing. But of the Boost library I expect a high-quality documentation. I think that these deficiencies in the documentation prevent a successful review of the library. I bet there are people who were put off by the docs and will not proceed to reviewing the library further, even though they could have given you many valuable insights, or point out defects. Some will stop at criticizing the docs. Any potential flaws in the library will not even be revealed. This may be the reason why things like ring buffer were not opposed: the potential critics may have not gotten that far. This is one of the reasons that I would recommend fixing the docs and staring the review again without the distraction. On the other hand, there is a lot of useful material in the documentation. For sure it is a way better that the original versions. For instance, the usage of ring buffer is documented clearly. Design. The idea to represent either a returned value or a reason for the lack thereof as `variant` is sound. Fixing the `E` to a single type is sound. using_error_code extended (along with ring buffer) for carrying payload is sound. I also like the fact that you only have three "monads": `outcome`, `result` and `expected`, and that they are maximally different: expected and result make the opposite decision on practically any aspect. This is a reasonable compromise between many possible combinations of micro-decisions, and the expectation to have a small number of types. `option` should go, I think. I can think of no use case for it. However, the design still changes throughout the review, and has not stabilized yet. The last message is that we may have varieties for narrow versus wide contract or we will have duplicated observers, and this has not been settled on yet. The consequence of using the ring buffer, where the error message can suddenly disappear is unfortunate, but then again, I know of no way to make it better. It might be the best choice of those available. I wish someone from low-latency users stepped in and commented on whether this is the right choice and whether the choice of these particular members (a short string, two unsigned ints) is the optimum in practice. Some aspects of design, to me seem unclear. E.g., what does it mean that `outcome` has value but I am calling `o.error()`? Is it a programmer error, which the function is trying to conceal, or does `o.error()` mean "convert `outcome` to `error_code`, whatever the state? Similarly, when `exception()` also recognizes error_codes, and `error()` does not recognize exception_ptrs looks quite random: I cannot see a mental model behind it. You could document detailed semantics of each function, but this would not be enough. I need a clear mental model, like "every error_code or exception_ptr is collectively called 'exception' in this library". If you do not support `outcome, I need a clear indication of it and a rationale. Implementation. These dual licensing, testing and git sub-module issues. I am not an expert in how Boost is organized, but a number of people I consider qualified have reported strong objections, and this has to be considered. A Boost library must be compliant with "the way things work in Boost" even if it is felt that things work wrong in Boost. I did not look much, at the implementation, but it looks acceptable on the surface. The assignment (most tricky part, I think) is implemented quite clearly. Sometimes static_assert's constrain too much. A non-default-constructible types should be allowed. Types that throw upon copy/move construction should be allowed provided I do not call the assignment on outcome/expected. I am told `outcome` does not work. I am not sure if this is fortunate. It is no longer an `outcome<T>` that works for any Regular T. What if I have a function that optains an error_code and might potentially fail. How can I express this in the interface? (Or do I have to wrap in `outcome>`?) `outcome` has function swap overloaded in namespace std. I am not sure if it is legal, but even if so, I would recommend implementing it in `outcome`'s namespace, in order not to mess with `std`. Again, thanks Niall for this library, and for a very educational discussion. Regards, &rzej;