Boost logo

Boost :

Subject: Re: [boost] [review] Review of Outcome (ends Fri-June-2)
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-05-30 21:50:25


I was not sure I would take the time to do a review, but given the
quantity of exchanges I believe that I must do it.

Le 29/05/2017 à 21:51, charleyb123 . via Boost a écrit :
> - What is your evaluation of the design?
Confusing. Functions with the same name do different things and what
they do is to wired for the specific case.

It is not clear if empty is a success or a failure of the computation. I
believe it is up to the user to decide what it is.
I'm not against having an empty state, but I would like to know at
looking at the type it is a success case or a failure without reading
the application interpretation of it.
I want an observer that tell me if the computation succeeds or fails.
I believe that has_value was the needed check, but all this depends on
what empty means.

I expected that error() returned the reason for failure, but I is not
the case, and it depends on the type.

I believe that even if people don't want operator* to have a narrow
contract we must be coherent with optional, and every smart pointer
(Possibly Value type).
I have no problem with the wide contracts, but we need a narrow one
doing the same thing so that we don't pay for what we don't use.
I don't believe that the narrow contracts need to be named with a
unsafe_ prefix or a _raw postfix.

Remember, we don't want to use unsigned int just because is is unsigned,
if we don't want overflow to be defined with modulo arithmetic. A bad
defined wide operation is not safer than a well defined narrow operation.

Functions with different purposes need different names. When you access
the error of a result<T> you don't expect to have a new error code in
case there was no error. This is error_or.
Maybe have some free function when this can be implemented using the
minimal interface. Maybe associated to a specific concept.
At the end, being "explicit is better than implicit".

I've not taken a look at all to the error_code_extended, but I believe
this is out of the scope of the library ans is something that could be
part of the Boost.System library.

The fact Niall don't wanted to depend on Boost (he has his reasons) has
as consequence that he reinvent the wheel. The behavior when there is no
exception is now the responsibility of Boost.Exception and any Boost
library that want to run with exception disabled must use this library.

Concerning boost-lite, I have no problem with whatever the library can
needs for implementation purpose or test to be in a detail folder. We
don't need sub-modules here.
When located in this folders this will be part of the implementation
evaluation, and not the design.

I don't know if the design of a class that must work when exceptions are
disabled, don't need really a different interface than a class that is
used with exceptions enabled.
I've not considered this while designing expected for the standard, as
there there is not this possibility. I recognize the utility of these
classes on such environments.
The definition of the operations must include this case (exceptions
disabled) and a rationale must be included to understand why this is the
best interface in both cases.
E.g. when exceptions are disabled, what is the meaning of value()? Do
we really want such operation in those environments?

As there are more than two possibilities, the visit() and an index()
function should be there so that we could avoid the if-then-else code.

The monad word should disappear from the doc and implementation. While
these classes are possible monads (or nested monads), the library
doesn't propose a monadic interface.

The TRY macro is not a monadic interface but a PossiblyValued interface.
What I mean is that the macros could work for any PossiblyValued type.
It could works for the proposed Outcomes classes as for std::optional,
std::experimental::expected. But Boost.Outcome has not this pretensions
and so the macro could be used only for the classes defined by Outcome,
as we don't have requirements on the types.

I wonder how the really functional monadic functions
(functor::transform, monad::chain, monad_error::catch_error) would be
defined for all these types, in particular for the types that have an
empty state and for those that have an error or an exception_ptr?
> - What is your evaluation of the implementation?
I've take a diagonal look at, and I believe that this is much more
complex than needed. I'll take a look next time if the design change.
I'm not saying that implementing this simple classes is simple. It is
not. I believe that Niall is right to look for a common class for all
this cases. However the internal class is not what I would like to have.

I' glad to see that Niall will remove all the preprocessor stuff.
I hope that he will adapt his library to Boost so that we need to read
less implementation details that are already in other libraries, ad
Boost.Config/Predef, Boost.BackTrace and Boost.Exception.
If there are missing macros in Boost.Config Niall should request/provide
new macros for Config/Predef.

At the end I believe we need a never-empty variant class that would help
to implement all these classes (something like what Anthony WIlliams
proposed for the standard).
I understand that Niall prefers to take in account only 2 or 3 specific
types, but I'm wondering if this restrictions are really helping to
define correctly the library. At least this makes the check by the user
more difficult.

> - What is your evaluation of the documentation?
I have read the documentation in diagonal. I don't think the
documentation describes clearly what is the motivation of this library,
what are the rationale for the design decisions and the reference
document is more than incomplete.
For a rationale for the design decisions I mean for each operation, in
which use cases this operation is interesting, what we cannot do without
it, ...

I've learn a lot about the library from the exchanges of this review.
What I have learn, was not evident in the documentation.

There are also references to the implementation details in the reference
documentation that make his lecture even more difficult.
There are serious problem with the Doxygen generated documentation? I'm
not again Doxygen, but we need to check it carefully.
> - What is your evaluation of the potential usefulness of the library?
I believe we need something like expected<T,E>, this is why I'm
proposing it for the standard, and maybe we need something more general.
I find the result/outcome to wired and I would prefer to have something
like result<success<T1, Tn>, failure<E1, ..., Em>> that make possible
all the proposed and to be proposed specific types. I don't think we
need too much specific cases as Niall is suggesting for the future
Outcome library. This result<success<T1, Tn>, failure<E1, ..., Em>>
would be a reasonable extension of expected<T,E> where T and E are in
some way variant<T1, ..., Tn>, variant<E1, ..., Em> respectively.

expected<variant<T1, ..., Tn>, variant<E1, ..., Em>> could with a
never-empty variant support all this as well, but there is some
syntactic sugar that is needed. If no syntactic sugar is needed we could
just use variant<T1, ..., Tn, E1, ..., Em>
but in this case there couldn't be a notion of success or failure of a
The proposed classes must make this separation clear, otherwise we will
be unable to provide in general a monadic interface for these classes.

As Andrzej signaled, the following are not the same

     expected<optional<T>, E>



Some private discussion with Niall and the discussion on this review
have raised points that could modify the possibly future expected class
in the standard. I say possible, because we never know.
While this could benefit Boost.Outcome only indirectly I hope that all
these open points will be discussed in the next C++ standard meeting.
> - Did you try to use the library? With what compiler? Did you have any
> problems?
Not at all. I would do it if I've found the good.
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
I've not counted the time. A lot I guess.
> - Are you knowledgeable about the problem domain?

Less than I could think before the review. People have multiple needs
and responding in a coherent way to all these needs is hard.
We should not forget to KISS, at least at the user level. Some believe
that UB is against them. It is not.
> And most importantly:
> - Do you think the library should be accepted as a Boost library?
No, not now.

I'm not for the proposed interface. I find the documentation not
addressing the problem and it is incomplete.

Maybe later, if Niall decides to go towards something simpler and more
coherent, ...
The same name for the same functionality, a different name when the
functionality changes.
I'm almost sure I don't want the 1st nor the 2nd HLS he his proposing.
Niall say that the global design has not hanged a lot between the
library we are reviewing and the one he is proposing to us if accepted.
He is right. He has accepted to suggestions but the global library
stands the same.
I believe however that his 2nd HLS could have more changes to be
accepted, but I don't think this could be a conditional acceptance.
Niall has a lot of things to do and the whole library should be reviewed.


Boost list run by bdawes at, gregod at, cpdaniel at, john at