[boost] [outcome] Formal review

Subject: [boost] [outcome] Formal review
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2017-06-01 01:27:44

With the extended review period ending soon (or perhaps slightly 
overdue, depending on timezone), and given my many posts regarding the 
library, I feel like I should probably express my thoughts in review 
form.  This is my first formal review and I am not myself a Boost 
library author (just a user) so please take that into account.

- What is your evaluation of the design?

A lot of thought has clearly gone into it, though I'm not sure I agree 
with some of the decisions made.

Empty state I'm on the fence about; I can see it's convenient both for 
optional<T> success return semantics and for not-yet-populated/called 
pending result semantics, but I'm not sure these are consistently 
applied as it stands, and they seem at least partly opposed to the 
"empty state is weird, let's aggressively throw" behaviour in the actual 
implementation.

Simplifying expected<T, E> into result<T> and outcome<T> makes sense, 
and the design of error_code_extended seems reasonable, including the 
content and ring buffer history.  However I wonder if perhaps some 
middle ground might be better, where result<T, E> and outcome<T, E> 
exist but E is constrained to a type with specific characteristics (such 
as being constructible from error_code and having noexcept move).  This 
might allow for more flexible error payloads.  Alternately some way to 
use a similar error_code_extended-linked-to-ring-buffer system but with 
a user-defined payload data type.  I don't consider this to be a defect 
or reason to reject the library, though, just something that would be 
useful if possible.

I don't like that result<error_code{,_extended}> is not legal, as this 
seems like unnecessary asymmetry, though again I don't consider this a 
rejection reason.

- What is your evaluation of the implementation?

I have not looked in depth at it.  The dual licensing and dual source 
distribution (preprocessed vs. not) bother me, but that will apparently 
be addressed in future.

I don't like the total duplication of interface methods (get vs. value, 
error vs. get_error), but this will apparently also be addressed in 
future.  (I am not opposed to having both narrow and wide accessors in 
the same type, though I prefer that the ones that can cause UB if abused 
require more typing.  I *am* opposed to having multiple names for the 
exact same behaviour.)

I'm not sure why it would be useful to provide const&& overloads of some 
of the value methods.  Usually the only time I've seen these is to 
explicitly delete that overload.  (Unless that is what's actually 
happening and doxygen is just not showing that correctly.)

I like the use of wide contracts by default, such that it is safe to 
call value(), error(), or exception() without inspecting the underlying 
type and they will react sensibly (eg. throwing) rather than having UB, 
although some of the finer points of what a sensible reaction means are 
still being debated.  But I can understand the desire of some people to 
have the other versions available for specific cases or in environments 
where static checking is more common.  Even so, I hope that the narrow 
versions will at least BOOST_ASSERT their preconditions.

The lack of use of Boost facilities such as Config, Assert, and Core 
(esp. BOOST_NO_EXCEPTIONS) bothers me more, and I do consider that a 
rejection reason.  Use of StackTrace would be good as well but at this 
point I don't consider that a defect since it's not quite "in" Boost 
yet.  (Possibly also Exception for boost::throw_exception, though using 
std::exception_ptr is the correct choice.)

There is a critical defect in error_code_extended's backtrace API, which 
has already been brought to the author's attention and should hopefully 
be resolved later.

- What is your evaluation of the documentation?

Generally good, although it seems to spend a lot of time and pages 
talking about expected<T, E>, and only towards the end adds "but you 
should probably use outcome<T> instead" as a sort of afterthought, which 
seems a bit backwards.

This is perhaps the correct order in a presentation, but it feels wrong 
in library documentation somehow.  I think it needs to be more up-front 
about outcome and result (since those are the core types of the library) 
and recommended usage, and cover the other things as background material 
afterwards.

The documentation refers to "monads" a lot, which is an unfamiliar 
concept to me.  Not sure if this needs more explanation or just rewording.

Doxygen's habit of putting hyperlinks that go nowhere in the reference 
pages is a bit disconcerting as well.

- What is your evaluation of the potential usefulness of the library?

Direct usefulness to me right now will be limited due to compiler 
support, but this seems like the sort of thing that would be quite 
useful in the future, especially to tidy up dual APIs such as those in ASIO.

They have a fairly good chance of becoming standard vocabulary types, 
which of course has also helped drive a lot of the discussion thus far.

- Did you try to use the library? With what compiler? Did you have any
problems?

I have not, as the compilers I use day-to-day are only C++11 or less, 
not C++14.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

Primarily a fairly detailed reading of the documentation, combined with 
discussion here.

- Are you knowledgeable about the problem domain?

Mixed.  I have experience with exception handling internals in MSVC and 
regularly work with software in both contexts of using error codes 
everywhere with exceptions banned, and of allowing exceptions but 
preferring they never actually be thrown.  I also regularly work with 
both low-latency and fixed-latency environments, in addition to more 
"normal" software.  But other than interaction with ASIO I have not made 
much use of error_codes to date, or mixing the two environments in the 
"sea of noexcept" fashion, though I can see the appeal.

- Do you think the library should be accepted as a Boost library?

Eventually, yes, but currently no (rejection, with encouragement to 
resubmit).

While the author claims that the actual implementation has changed 
little since initial submission, the changelog in the "review accepted 
feedback" threads sounds fairly serious, and I'm not sure I like some of 
its directions (such as separate types for contract variations). 
Perhaps it will indeed end up being no big deal, but I don't feel like I 
have a good handle on what the end result will look or behave like so I 
don't feel comfortable endorsing it until I can get a better look at it.

Primarily though I think the Boost integration was done backwards; the 
library should make use of Boost facilities first (as previously 
mentioned); if also distributed separately then that separate 
distribution should have the Boost emulation layer in it, and not the 
reverse.

(To clarify: using std:: facilities is preferred, so using 
<system_error> is better than using <boost/system/system_error.hpp>, 
although it would be nice if compatible with the latter without actually 
depending on it unless requested explicitly.  But the main problem is 
lack of use of Config, Assert, and Core, unless I missed something.)