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

Subject: [boost] [outcome] Second high level summary of review feedback accepted so far
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-05-30 14:58:22

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:

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>

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.

2. I have been persuaded to replace the implicit conversion from
static_checked_* varieties to runtime_checked_* varieties with explicit
conversion instead as the implicit conversion could produce runtime
failure where the programmer did not expect that to occur.

Regarding other changes that I have accepted since the previous high
level summary:

1. Outcome's STL exception throw catch to result<T> super-macro was
found to be overkill given that current compilers won't replace simple
throw...catch sequences with a branch, and still invoke the full
throw-catch runtime machinery whatever the case. Therefore an exception
to error code conversion function makes more sense than a macro.
https://github.com/ned14/boost.outcome/issues/50

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.

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

4. Given where Vicente appears to be heading next with expected<T, E1,
E2, ...> it may be wise to make the static_checked_* varieties use
std::variant<...> for storage when being compiled on C++ 17.
https://github.com/ned14/boost.outcome/issues/53

Thanks,
Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/