Subject: Re: [boost] [outcome] Formal review
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2018-01-29 00:04:29
> 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.
Agreed, and already tracked at https://github.com/ned14/outcome/issues/110.
> 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.
> There may be an overemphasis on constexpr; I would rather have had
> support for C++11.
Early on during the rewrite, I did a quick hack out of the constexpr to
check if C++ 11 was feasible. The problem is that the older compilers
tend to ICE badly with Outcome even without constexpr involved, so even
if it were C++ 11 compatible, it would probably still need the very
recent versions of the compilers it currently does. I felt therefore one
might as well take advantage of C++ 14 if the minimum compiler versions
implement that well.
> 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.
Sigh. Great spot. Logged to https://github.com/ned14/outcome/issues/120.
> 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.
A known limitation of Standardese. Tracked by
> 2. What are the requirements for EC and EP?
is the requirements predicate. It ought to be documented. Tracked by
> 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.
Logged to https://github.com/ned14/outcome/issues/122
> 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.
As I'm sure you know, there are subtle bugs in the implementation of
that proprietary extension on some compilers. Hence I deliberately
listed the implementation.
> 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 believe Jonathan has since made Standardese more conforming to how
WG21 specifications are worded.
> Â 2. People must be able to submit patches to the Boost version of the
> Â Â Â Â library without having to convert those patches to the standalone
> Â Â Â Â version.
That's no problem, the regexs which convert Outcome into Boost.Outcome
also run safely in the opposite direction, which is by design.
Thanks for the review!
-- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk