With the extended review period ending soon (or perhaps slightly
overdue, depending on timezone), and given my many posts regarding the
library, I feel like I should probably express my thoughts in review
form. This is my first formal review and I am not myself a Boost
library author (just a user) so please take that into account.
- What is your evaluation of the design?
A lot of thought has clearly gone into it, though I'm not sure I agree
with some of the decisions made.
Empty state I'm on the fence about; I can see it's convenient both for
optional<T> success return semantics and for not-yet-populated/called
pending result semantics, but I'm not sure these are consistently
applied as it stands, and they seem at least partly opposed to the
"empty state is weird, let's aggressively throw" behaviour in the actual
implementation.
Simplifying expected into result<T> and outcome<T> makes sense,
and the design of error_code_extended seems reasonable, including the
content and ring buffer history. However I wonder if perhaps some
middle ground might be better, where result and outcome
exist but E is constrained to a type with specific characteristics (such
as being constructible from error_code and having noexcept move). This
might allow for more flexible error payloads. Alternately some way to
use a similar error_code_extended-linked-to-ring-buffer system but with
a user-defined payload data type. I don't consider this to be a defect
or reason to reject the library, though, just something that would be
useful if possible.
I don't like that result is not legal, as this
seems like unnecessary asymmetry, though again I don't consider this a
rejection reason.
- What is your evaluation of the implementation?
I have not looked in depth at it. The dual licensing and dual source
distribution (preprocessed vs. not) bother me, but that will apparently
be addressed in future.
I don't like the total duplication of interface methods (get vs. value,
error vs. get_error), but this will apparently also be addressed in
future. (I am not opposed to having both narrow and wide accessors in
the same type, though I prefer that the ones that can cause UB if abused
require more typing. I *am* opposed to having multiple names for the
exact same behaviour.)
I'm not sure why it would be useful to provide const&& overloads of some
of the value methods. Usually the only time I've seen these is to
explicitly delete that overload. (Unless that is what's actually
happening and doxygen is just not showing that correctly.)
I like the use of wide contracts by default, such that it is safe to
call value(), error(), or exception() without inspecting the underlying
type and they will react sensibly (eg. throwing) rather than having UB,
although some of the finer points of what a sensible reaction means are
still being debated. But I can understand the desire of some people to
have the other versions available for specific cases or in environments
where static checking is more common. Even so, I hope that the narrow
versions will at least BOOST_ASSERT their preconditions.
The lack of use of Boost facilities such as Config, Assert, and Core
(esp. BOOST_NO_EXCEPTIONS) bothers me more, and I do consider that a
rejection reason. Use of StackTrace would be good as well but at this
point I don't consider that a defect since it's not quite "in" Boost
yet. (Possibly also Exception for boost::throw_exception, though using
std::exception_ptr is the correct choice.)
There is a critical defect in error_code_extended's backtrace API, which
has already been brought to the author's attention and should hopefully
be resolved later.
- What is your evaluation of the documentation?
Generally good, although it seems to spend a lot of time and pages
talking about expected, and only towards the end adds "but you
should probably use outcome<T> instead" as a sort of afterthought, which
seems a bit backwards.
This is perhaps the correct order in a presentation, but it feels wrong
in library documentation somehow. I think it needs to be more up-front
about outcome and result (since those are the core types of the library)
and recommended usage, and cover the other things as background material
afterwards.
The documentation refers to "monads" a lot, which is an unfamiliar
concept to me. Not sure if this needs more explanation or just rewording.
Doxygen's habit of putting hyperlinks that go nowhere in the reference
pages is a bit disconcerting as well.
- What is your evaluation of the potential usefulness of the library?
Direct usefulness to me right now will be limited due to compiler
support, but this seems like the sort of thing that would be quite
useful in the future, especially to tidy up dual APIs such as those in ASIO.
They have a fairly good chance of becoming standard vocabulary types,
which of course has also helped drive a lot of the discussion thus far.
- Did you try to use the library? With what compiler? Did you have any
problems?
I have not, as the compilers I use day-to-day are only C++11 or less,
not C++14.
- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
Primarily a fairly detailed reading of the documentation, combined with
discussion here.
- Are you knowledgeable about the problem domain?
Mixed. I have experience with exception handling internals in MSVC and
regularly work with software in both contexts of using error codes
everywhere with exceptions banned, and of allowing exceptions but
preferring they never actually be thrown. I also regularly work with
both low-latency and fixed-latency environments, in addition to more
"normal" software. But other than interaction with ASIO I have not made
much use of error_codes to date, or mixing the two environments in the
"sea of noexcept" fashion, though I can see the appeal.
- Do you think the library should be accepted as a Boost library?
Eventually, yes, but currently no (rejection, with encouragement to
resubmit).
While the author claims that the actual implementation has changed
little since initial submission, the changelog in the "review accepted
feedback" threads sounds fairly serious, and I'm not sure I like some of
its directions (such as separate types for contract variations).
Perhaps it will indeed end up being no big deal, but I don't feel like I
have a good handle on what the end result will look or behave like so I
don't feel comfortable endorsing it until I can get a better look at it.
Primarily though I think the Boost integration was done backwards; the
library should make use of Boost facilities first (as previously
mentioned); if also distributed separately then that separate
distribution should have the Boost emulation layer in it, and not the
reverse.
(To clarify: using std:: facilities is preferred, so using
is better than using ,
although it would be nice if compatible with the latter without actually
depending on it unless requested explicitly. But the main problem is
lack of use of Config, Assert, and Core, unless I missed something.)