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

Subject: Re: [boost] [outcome] Second high level summary of review feedback accepted so far
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2017-05-30 21:38:23

2017-05-30 16:58 GMT+02:00 Niall Douglas via Boost <boost_at_[hidden]>:

> Firstly thanks to everyone who has contributed no less than **618**
> emails at the time of writing to the Outcome review. Despite some on
> Reddit calling this review a "train wreck" and indeed some lovely things
> about me personally, I have found this review deeply helpful, and I
> believe so have most of its contributors in helping us all firm up our
> differing opinions about what such a vocabulary type ought to be.
>
>
> With regard to the previous high level summary at
> http://boost.2283326.n4.nabble.com/outcome-High-level-
> summary-of-review-feedback-accepted-so-far-tp4694767.html,
> I have made only these changes to the changes in that summary:
>

Thanks Niall, for the summary. But now, after these changes, the review I
am still writing is useless, as it applies to a different library. Maybe we
should schedule another review?

>
> 1. I have been persuaded to use longer more appropriate naming for
> result<T> and outcome<T>, so now the typedefed varieties with implicit
> conversions to their empty-capable form indicated by "=>" are:
>
> - static_checked_outcome<T> => static_checked_optional_outcome<T>
>   static_checked_result<T>  => static_checked_optional_result<T>
>

No, no. I strongly suggest to provide only one outcome, with empty state if
needed, and both narrow and wide contracts. We have learned to live with
such types in C++, even if they are far from perfect.

>
> These have narrow observers which reinterpret_cast and are therefore
> undefined behaviour if the state does not match, but use
> __builtin_unreachable() to help static analysis tooling like clang-tidy
> spot UB where it can be statically deduced. Conceptually, they are a
> very thin convenience wrap of std::variant<...>.
>
>
> - runtime_checked_outcome<T> => runtime_checked_optional_outcome<T>
>   runtime_checked_result<T>  => runtime_checked_optional_result<T>
>
> The runtime_checked_optional_* varieties are identical and unchanged to
> the outcome<T> and result<T> in the submitted library. They have wide
> observers with a formal empty state and it is never undefined behaviour
> to call any observer. This allows the programmer to save typing
> considerable boilerplate knowing that the runtime checks save the
> programming needing to spell things out. I have not been persuaded to
> change a single thing in their design yet apart from removing
> unnecessary member functions, but thanks to everybody who has tried, and
> if you think I am making a critical mistake, you still have a few days
> left to persuade me of it!
>
> For anyone not familiar with the discussion and feels that the names are
> excessively long, be aware they are just convenience typedefs of a more
> complex to configure "template<...> class outcome_impl". It is expected
> that end users of the library will choose the varieties they want for
> their code and typedef them into their local namespace as "result<T>" or
> whatever. Nobody is proposing that end users actually type out the full
> name each and every time they use them, and the documentation will make
> that clear.
>

>
>
>
> Regarding other changes that I have accepted since the previous high
> level summary:
>
> 2. Outcome's usage of git submodules is felt by some reviewers to be a
> showstopper for acceptance as they could bring in non-Boost code
> unexpectedly in the future. I have agreed to make a cronjob script
> generated Boost.Outcome from standalone Outcome in the same fashion as
> Boost.ASIO is script generated from standalone ASIO.
> https://github.com/ned14/boost.outcome/issues/51
>
> - Licensing to be BSL only
> - No cmake, just bjam
> - No git submodules at all
> - std-flavoured, not boost-flavoured
> - No code to be brought in except that written by me i.e. a hardcoded
> whitelist applies.
> - Contents of Boost repo to only contain material directly relevant to
> Boost. Nothing else.
>

Thanks. This will make the acceptance easier.

> 3. Under the assumption that error_code_extended's use of a static
> global ring buffer would be controversial in this review, I hacked a
> quick and dirty solution expecting to have to remove it. Now we know
> that few disapprove, a more serious implementation will be needed.
> https://github.com/ned14/boost.outcome/issues/52

I am not sure of that. It was my impression that many potential reviewers
were put off by documentation and legal/structural issues, and stopped with
their review at that point. Once you have move thes obstacles out of the
way, you will likely draw more reviewers, and who knows what they have to
say about the ring buffer.

Regards,
&rzej;