Subject: [boost] [outcome] Formal review
From: Bjorn Reese (breese_at_[hidden])
Date: 2018-01-28 17:12:08
Boost.Outcome provides a useful alternative / complement to exceptions,
and its design is clean.
I like naming of functions with wide and narrow contracts.
I suggest that result<> is renamed to basic_result<>, which will then
be the customizable template class, and result<> then becomes as
specialzation (template alias) that does the same as the current default
result<>. This makes result<> an uncustomizable companion to
std::error_code, which is useful when defining interoperable APIs.
One design decision that I disagree with is the choice of sum types
rather than product types for internal storage. I would rather have a
lower run-time footprint at the expense of increased compile times.
Looks readable and solid.
There may be an overemphasis on constexpr; I would rather have had
support for C++11.
BOOST_OUTCOME_TRY_UNIQUE_NAME generates identifiers starting with __t
but according to the C++ standard [lex.name/3.1] "each identifier that
contains a double underscore __ [...] is reserved to the implementation
for any use", where implementation means the C++ compiler.
The documentation is well-written, with a tutorial that is easy to
The API Reference could be improved though.
1. What are the requirements for ValueOrError? Looking up ValueOrError
brings us to convert::value_or_error, which only states that it
"matches the ValueOrError concept." The rather convoluted enable_if
uses ValueOrError<U> which uses detail::ValueOrError<U> and here the
trail disappears. The convert.hpp header file does contain the
missing explanations though.
2. What are the requirements for EC and EP? EC and EP are used
informally throughout the documentation, but are described in the
Custom Payloads tutorial. In the API Reference they are suddenly
called S and P instead. Please use a consistent naming throughout the
documentation; preferably using more descriptive names, such as
OutcomeErrorCode and OutcomeExceptionPtr. It would furthermore be
helpful if they were described as concepts outside the tutorial.
3. The descriptions of value_type_if_enabled and error_type_if_enabled
are obscure, and it is still not clear to me if they have any
relevance for the library user. If they do, then their descriptions
should be more clear; if they do not, then they should not be
included in the documentation.
4. The documentation of OUTCOME_TRYX contains the full macro
implementation which is not really helpful to users. This macro could
possibly be expressed in terms of other macros.
5. noexcept('hidden') is used in several places. Does this mean that the
noexcept condition is unspecified? If so, then it is more customary
to use "unspecified" in italic.
I spent 4-5 hours reading the documentation and looking at the code. I
did not compile it.
I vote for the conditional acceptance of Boost.Outcome.
The conditions are:
1. The documentation should be integratable into the Boost
2. People must be able to submit patches to the Boost version of the
library without having to convert those patches to the standalone
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk