Re: [boost] [outcome] High level summary of review feedback accepted so far

Subject: Re: [boost] [outcome] High level summary of review feedback accepted so far
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-05-27 07:55:57

Le 27/05/2017 à 02:09, Niall Douglas via Boost a écrit :
> Some reviewers have asked what form Outcome will take in the near
> future. The following is a high level summary of the changes already
> made on develop branch or will be made according to
> https://github.com/ned14/boost.outcome/issues logged from review
> feedback. I have left out some minor issues, this lists just the major
> changes. This should help reviewers to decide whether to recommend
> acceptance or rejection. If there are any questions, please do ask.
>
Thanks Niall for this summary
> Major design changes:
> =====================
> - option<T> to be removed https://github.com/ned14/boost.outcome/issues/48
>
>
> - outcome<T> and result<T> will have their empty state removed, and all
> observers gain narrow contracts. Default construction is disabled.
> https://github.com/ned14/boost.outcome/issues/44. Example:
>
> ```
> result<Foo> v(make_errored_result(std::errc::invalid_argument));
> assert(v.has_error());   // true
> assert(!v.has_value());  // true
> // As if Foo(reinterpret_cast<Foo &&>
> //  (error_code_extended(std::errc::invalid_argument));
> Foo f(std::move(v.value()));
> assert(v.has_error());   // still true
> assert(!v.has_value());  // still true
> ```
Just a question I could do for the existing library also. What has_error 
mean for outcome, result? is that it has an EC or that it has no value?
And now that we don't have empty, what is the sens of has_error for result?
Maybe, outcome should have a get_state function that returns an enum so 
that the user can do a switch.

> (NOTE: expected<T, E> will track whatever LEWG Expected does, but it
> differs from what will become result<T> by having a wide contract on
> .value() and narrow contracts on operator*(), .error(), operator->().
> result<T> will have narrow contracts on everything, it is basically a
> thin wrap of std::variant<T, error_code_extended> except with strong
> never empty warranty)
>

What will be the differences between result<T> and expected<T, 
error_code_extended>?

The wide contracts for the observers? Can not we provide wide and narrow 
contracts or don't reuse the same name with different meaning?

If we had a expected<T, E1, .., En> what will be the differences between 
outcome<T> and expected<T, error_code_extended, exception_ptr>?
> - New typedefs outcome_e<T> and result_e<T> are identical to outcome<T>
> and result<T> except for adding a formal empty state. Observer contract
> slightly widens, an attempt to use an empty object throws a
> bad_outcome_access exception. Implicit conversion from non-empty-capable
> varieties is permitted to empty-capable varieties, but not the other way
> round. Default construction is to **empty**.
> https://github.com/ned14/boost.outcome/issues/44
Okay this corresponds to what others are naming optional_outcome, 
optional_result.

If we had a optional_expected<T, E1, .., En>
what will be the differences between result_e<T> and 
optional_expected<T, error_code_extended>?
what will be the differences between outcome_e<T> and 
optional_expected<T, error_code_extended, exception_ptr>?
>
>
> - New typedefs checked_outcome<T>/checked_result<T>,
> checked_outcome_e<T>/checked_result_e<T> are added. These mirror the
> editions just described, but checks and default actions occur on all
> observer usage so hidden reinterpret_cast<> never occurs. Implicit
> conversion from non-checked varieties is permitted to checked varieties,
> but not the other way round.
> https://github.com/ned14/boost.outcome/issues/47. Examples:
>
> ```
> // Note result<T> implicitly converts to checked_result<T>, but not
> // the other way round. So we can use same make_errored_result().
> checked_result<Foo> v(make_errored_result(std::errc::invalid_argument));
> assert(v.has_error());   // true
> assert(!v.has_value());  // true
> // .value() throws std::system_error(
> //   std::make_error_code(std::errc::invalid_argument));
> Foo f(std::move(v.value()));
> ```
>
> ```
> checked_result<Foo> v(make_valued_result(Foo()));
> assert(!v.has_error());  // true
> assert(v.has_value());   // true
> // .error() returns a default constructed (null) error_code_extended
> // when result is valued to indicate "no error here"
> error_code_extended ec(std::move(v.error()));
> assert(!ec);             // true
> ```
I will need more rationale about the need of this classes and why we 
need to do an action while observing them. Please, could you elaborate?

> - The presented library self generates using lots of preprocessor based
> multiple #include's to stamp out finished editions of implementation.
> The git repo partially preprocesses this into a nearly finished edition
> for minimum compile time load for end users.
>
> Due to the many more varieties of outcome<T> and result<T> provided, the
> preprocessor machinery would be replaced with a template based machinery
> instead. I will look into making optional use of extern template and a
> static library to retain minimum compile time load, but header only
> usage will remain possible.
Glad to hear you will remove the pre-processor and use just generic 
templates.

My view is that the generic classes do we need are

expected<T, E1, ..., EN>

and

optional_expected<T, E1, ...En>

We could limit them to 1 or 2 Error arguments if we want as this is what 
we need for now.
>
>
> Smaller design changes:
> =======================
> - tribool stuff to go away in default configured build.
> https://github.com/ned14/boost.outcome/issues/22
>
> - .get*() functions will be removed.
> https://github.com/ned14/boost.outcome/issues/24
>
> - Don't include <windows.h> on winclang.
> https://github.com/ned14/boost.outcome/issues/39

>
> Documentation changes:
> ======================
> - Licence boilerplate missing on some files.
> https://github.com/ned14/boost.outcome/issues/38
>
> - Reference API docs need to spell out exact preconditions,
> postconditions and semantics per API.
> https://github.com/ned14/boost.outcome/issues/26

I will say, not only the pre-conditions, but what the function does, 
requires (statically) what returns, whether it throws and what, SFINAE, 
exception safety, what are the constraints on the template parameters, ...
Otherwise we don't know what the function does and where it can be used.
>
> - Reference API docs need to describe types void_rebound, *_type and
> raw_*_type in detail. https://github.com/ned14/boost.outcome/issues/29
I don't know what this is used for yet, would you need this after the 
refactoring?
>
> - User overridable macros which throw exceptions and provide
> customisation points have vanished from the docs somehow.
> https://github.com/ned14/boost.outcome/issues/35
What about the use of BOOST_THROW_EXCEPTION?
>
> - Landing page and introduction to be broken up into bitesize single
> page chunks. https://github.com/ned14/boost.outcome/issues/41
>
>
> Still to be decided:
> ====================
> - Should *_e() varieties provide convenience .get(), .get_error(),
> .get_exception() which returns T, error_code_extended and
> std::exception_ptr by value moved from internal state, resetting state
> afterwards to empty? These would mirror future.get()'s single shot
> observers.
>

We need a valid use case to introduce them and even more as member 
functions.
In any case, these functions can be defined on to of the provided 
interface, and could be non-member functions, isn't it?

Vicente