Re: [boost] [review] Review of Outcome (ends Fri-June-2)

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

Hi,

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 
computation.
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>

and

     optional<expected<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.

Best,
Vicente