[outcome] Formal review
I. DESIGN --------- 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. II. IMPLEMENTATION ------------------ 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. III. DOCUMENTATION ------------------ The documentation is well-written, with a tutorial that is easy to understand. 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. IV. MISC -------- I spent 4-5 hours reading the documentation and looking at the code. I did not compile it. V. VERDIC --------- I vote for the conditional acceptance of Boost.Outcome. The conditions are: 1. The documentation should be integratable into the Boost infrastructure. 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.
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_s... 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/
participants (2)
-
Bjorn Reese -
Niall Douglas