What follows is my review of the Outcome library.
I. DESIGN
---------
The general idea of an error handling framework is sound, extremely
appealing, and I found it well explained in the documentation. I like the
concept very much and agree with many of the design decisions taken
throughout. Specifically, constraining the error type to be the fixed
std::error_code (or equivalent) and providing a wide accessor interface.
I don't think that option<T> should have been included at all. It duplicates
optional<T> and has no good reason to exist other than to showcase that the
underlying policy-based implementation can produce it.
I don't consider expected a particularly needed part of the library;
the focus should be on result<T> and outcome<T>, the two classes that
represent the error handling philosophy on which the library is built.
As already stated, I consider the interface of result<T> and outcome<T> a
bit too cluttered. Not everyone shares my design preferences for minimal but
complete interfaces, so there's legitimate argument to be had over whether,
for instance, operator*, operator-> or value_or ought to be included, but
the needless duplication between get() and value() is, well, needless.
For an initial submission, it's better to err on the side of too little,
rather than too much, because it's easy to add later, hard to remove.
The empty state has been a source of controversy, and with reason. My
initial stance was that it simply needs to go. This however presents a
legitimate design question as to what the behavior of the default
constructor ought to be. After seeing the suggestions of leaving the object
uninitialized there, I now see the empty state as a significantly lesser
evil. Having an empty state without the default constructor putting the
object into it however is pointless.
I do not agree with the direction things have taken of providing a multitude
of result classes instead of a single one. Components that are intended to
be used in interfaces should provide one, carefully designed option, which
encourages what the designer deems best practices. The idea here, assuming
that we shoot for standard acceptance at some point, is to provide
std::result<T> and have APIs return it, not provide a bag of options and let
everyone pick its own thing. (They already do that today and we're not
better for it.)
I do not agree with making value() unchecked by default. As I explained, one
expression of the error handling philosophy behind the library is the
following snippet:
auto r = function().value();
where function() is noexcept and returns result<T>, whereas the place where
the above line appears is a higher level component that signals errors not
via error_code, but via exceptions. This one-liner provides the transition
in an elegant way; losing it obscures the vision behind the library.
Instead of using std::error_code, the library provides its own
error_code_extended, complete with an interesting and very useful ring
buffer logging mechanism that stores additional information along with the
standard code/category values. Specifically, it returns error_code_extended
from result<T>::error().
I would separate things a bit further here, attaching the additional
information not to a separate error_code_extended class, but to result<>
itself. That is, result<>::error() would return std::error_code, and I'd
provide a separate accessor, result<>::error_info(), say, that would return
the ring buffer entry associated with this result<>. This for me provides a
cleaner separation between the basic result<> functionality, and the
extended one, although this is a matter of taste.
I would also support chaining result<>s by storing the index of the parent
ring buffer entry in the current ring buffer entry. This is a
straightforward and very useful extension.
Instead of providing a macro BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT, I
would provide
result<T>::set_error_from_exception(std::exception const& e);
and
outcome<T>::set_error_from_exception(std::exception const& e,
std::exception_ptr const & p);
that would do more or less what the macro does, except the fallback in the
second case would be to store p instead of (EINVAL, e.what()).
I'd also use make_error_result and make_value_result instead of
make_errored_result and make_valued_result, as the former seem more
grammatically correct.
II. IMPLEMENTATION
------------------
The implementation, from a cursory look, appears of high quality, which is
not surprising given that it was in use for two years. The preprocessed
header is not easy to review, but the non-preprocessed form is available and
amenable to study.
Simple examples using outcome<T> compile and work well, although under
VS2017 Clang/C2 windows.h is included, which is unfortunate but fixable.
The library does not provide a test/Jamfile, which means that as submitted
it will not integrate into Boost's testing infrastructure. Given that a
Jamfile that performs the equivalent of withmsvc.bat is trivial to write:
import testing ;
project : requirements <include>../include/boost/outcome/v1.0 ;
run unittests.cpp ;
the omission is inexplicable. (Further integration into the Boost test
matrix would ideally entail splitting unittests.cpp into separate .cpp tests
so that success/failure can be tracked independently.)
With the above Jamfile, trying to test the library with Cygwin gcc fails
with:
D:\tmp2\boost-master\libs\outcome\test>b2 toolset=gcc-cxx14
[...]
gcc.compile.c++
..\..\..\bin.v2\libs\outcome\test\unittests.test\gcc-cxx14-6.3.0\debug\unittests.o
In file included from ../include/boost/outcome/outcome.hpp:26:0,
from unittests.cpp:34:
../include/boost/outcome/outcome_v1.0.hpp:1446:22: fatal error: execinfo.h:
No such file or directory
#include
^
compilation terminated.
I realize that Cygwin is not a supported platform, but this is what I have
here and this is what I test with, so I figured I'll give it a try.
Trying to test with VS 2017 fails with:
D:\tmp2\boost-master\libs\outcome\test>b2 toolset=msvc-14.1
[...]
compile-c-c++
..\..\..\bin.v2\libs\outcome\test\unittests.test\msvc-14.1\debug\threading-multi\unittests.obj
unittests.cpp
unittests.cpp(780): error C3245:
'boost::outcome::_1_0_std_std_9274c0d4::basic_monad::is_constructible':
use of a variable template requires template argument list
d:\tmp2\boost-master\libs\outcome\include\boost\outcome\outcome_v1.0.hpp(4271):
note: see declaration of
'boost::outcome::_1_0_std_std_9274c0d4::basic_monad::is_constructible'
unittests.cpp(780): fatal error C1903: unable to recover from previous
error(s); stopping compilation
I get the same failure with /std:c++latest.
Testing with the msvc-14.0 toolset works.
The library as submitted contains both a preprocessed, fully self-contained,
single header named outcome_v1_0.hpp, which is used by default, and a
traditional non-preprocessed form that can be chosen with a macro. For the
non-preprocessed form to be functional, a git submodule is incorporated by
reference:
[submodule "include/boost/outcome/boost-lite"]
path = include/boost/outcome/boost-lite
url = https://github.com/ned14/boost-lite.git
branch = master
fetchRecurseSubmodules = true
ignore = untracked
which in turn incorporates two more submodules:
[submodule "include/gsl-lite"]
path = include/gsl-lite
url = https://github.com/martinmoene/gsl-lite.git
branch = master
fetchRecurseSubmodules = true
ignore = untracked
[submodule "pcpp"]
path = pcpp
url = https://github.com/ned14/pcpp.git
branch = master
fetchRecurseSubmodules = true
ignore = untracked
The first of these is not by the library author, and the second is a derived
work.
It is not possible for a Boost release, say, boost_1_65_0.zip, to
incorporate git submodules by reference, so there remains a question of
what, exactly, is being submitted for inclusion. My attempts to clarify this
with the author have proven unsuccessful.
I consider this a critical defect of the submission. It is simply not
possible for a reviewer to give an opinion whether the submission ought to
be included into Boost releases without it being known what, specifically,
is being proposed to be included.
There is no problem with the library containing implementation details under
its directory, but we need to know what these details are.
What the library actually needs as dependencies from boost-lite is not much
and it's perfectly possible to extract the necessary parts and prepare a
self-contained submission. This should have been done prior to the review.
The files in the library contain the following:
```
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License in the accompanying file
Licence.txt or at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Distributed under the Boost Software License, Version 1.0.
(See accompanying file Licence.txt or copy at
http://www.boost.org/LICENSE_1_0.txt)
```
This intends to dual license the library, but as written, it doesn't. What
it does is apply the terms of BOTH licenses to distribution and use. This is
also a critical defect. If the library is included in a Boost release, it
should make it perfectly clear that it's licensed under the Boost license,
and not under the intersection of Boost and Apache 2.0.
There are several ways to do it, one is to clarify the above to specifically
say "ASL2 or BSL, at your option" in the appropriate legalese, another is to
license the Boost-included Outcome under BSL and the standalone Outcome
under whatever. My recommendation would be to just license the lot under BSL
and be done with it. Dual licensing creates unnecessary legal complications
and may, from a legal point of view, be more burdensome than just using the
BSL. Contributors need to be aware that they are licensing their patches
under both licenses, it might not be very clear whether they have been
aware, and so on.
At times, the library duplicates certain functionality present elsewhere in
Boost. I do not consider this a defect. It's a legitimate design decision to
avoid intra-Boost dependencies, although the author needs to be aware of the
downsides - using Boost components is basically taking advantage of free
work instead of having to maintain everything oneself.
III. DOCUMENTATION
------------------
I found the documentation perfectly adequate and was able to get a very good
outline of how the classes were designed, how they were intended to be used,
and what problem they intend to solve. It focuses a bit too much on
expected, which is not in itself a bad thing but is, for me, not
exactly what the library is about. Fixing E to std::error_code is a
fundamental (and correct) design decision from which these classes derive
much of their utility.
The Doxygen-generated reference will not win any awards, but it does the
job. It however contains too many implementation details that would ideally
either not be there at all, or tucked away in their own separate section.
The monadic content is also a bit too high for my taste, and I would prefer
if a user of result<> and outcome<> does not encounter the word monad at
all.
IV. VERDICT
-----------
Should Outcome be accepted?
In short, eventually. This, at present, is a NO.
Ideally, what I would like to see is the library resubmitted in a
conventional form, consisting of exactly the files that are intended to go
into the libs/outcome directory in boost_1_65_0.zip, licensed under the BSL,
only containing the files necessary for the non-preprocessed version of the
code to compile and work. In addition, the test/ subdirectory would contain
a Jamfile that is ready to integrate into the Boost test matrix.
