Boost logo

Boost :

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/

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk