Re: [boost] [outcome] Formal review

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.

Understood.

> 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.
Thanks.

> 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
https://github.com/foonathan/standardese/issues/58 and
https://github.com/foonathan/standardese/issues/76.

> 2. What are the requirements for EC and EP?

Good point.
https://github.com/ned14/outcome/blob/master/include/outcome/detail/result_storage.hpp#L164
is the requirements predicate. It ought to be documented. Tracked by
https://github.com/ned14/outcome/issues/121.

> 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!

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/