Re: [boost] [outcome] Review of Outcome

Subject: Re: [boost] [outcome] Review of Outcome
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-05-28 23:25:30

> The general idea of an error handling framework is sound, extremely
> appealing, and I found it well explained in the documentation. I like
> the concept very much and agree with many of the design decisions taken
> throughout. Specifically, constraining the error type to be the fixed
> std::error_code (or equivalent) and providing a wide accessor interface.
> [snip]
> I don't consider expected<T, E> a particularly needed part of the
> library; the focus should be on result<T> and outcome<T>, the two
> classes that represent the error handling philosophy on which the
> library is built.

FYI about half my potential user base want to set type E to their own
type, and see setting E to error_code as highly retrograde because it
loses type safety enforcement of disparate error code domains.

It's why I implemented Expected. It doubles my potential user base easily.

> I do not agree with the direction things have taken of providing a
> multitude of result classes instead of a single one. Components that are
> intended to be used in interfaces should provide one, carefully designed
> option, which encourages what the designer deems best practices.

As we have discussed, I prefer a competitive environment of
alternatives. But if Vicente and I can come to agreement on naming of
wide vs narrow observers, then we will only have:

- result<T> and outcome<T> - without empty state
- optional_result<T> and optional_outcome<T> - with empty state

> The
> idea here, assuming that we shoot for standard acceptance at some point,
> is to provide std::result<T> and have APIs return it, not provide a bag
> of options and let everyone pick its own thing. (They already do that
> today and we're not better for it.)

I would recommend against standardising result<T> and outcome<T> in the
strongest terms. Expected is the right design for the STL, not Outcome.

I hope for SG14 to include Outcome in their standards quality collection
of recommended libraries for low latency users, but in the
non-standards-track category.

> I would separate things a bit further here, attaching the additional
> information not to a separate error_code_extended class, but to result<>
> itself. That is, result<>::error() would return std::error_code, and I'd
> provide a separate accessor, result<>::error_info(), say, that would
> return the ring buffer entry associated with this result<>. This for me
> provides a cleaner separation between the basic result<> functionality,
> and the extended one, although this is a matter of taste.
> 
> I would also support chaining result<>s by storing the index of the
> parent ring buffer entry in the current ring buffer entry. This is a
> straightforward and very useful extension.

It is an interesting idea, but as I said at the time not as useful as
you might think given the ephemeral nature of the storage.

I am still surprised that nobody has yet objected strongly to storage
which can vanish randomly during usage.

> Instead of providing a macro BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT, I
> would provide
> 
>  result<T>::set_error_from_exception(std::exception const& e);
> 
> and
> 
>  outcome<T>::set_error_from_exception(std::exception const& e,
> std::exception_ptr const & p);
> 
> that would do more or less what the macro does, except the fallback in
> the second case would be to store p instead of (EINVAL, e.what()).

That's a great idea for an API, and a variation on that has been logged
to https://github.com/ned14/boost.outcome/issues/50.

You may not have realised the main use case for
BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT: the elision under optimisation
of C++ exception throws when using STL code. So for example:

```
result<void> function(std::vector<Foo> &a) noexcept
{
  try
  {
    a.push_back(Foo());
  }
  BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT
}
```

Here vector.push_back() might throw a std::bad_alloc. Under
optimisation, the above code converts the throw of std::bad_alloc into a
branch to early exit of an errored result<void>. So the exception throw
and catch machinery is completely removed, and replaced with a fast
branch. A very big win.

> I'd also use make_error_result and make_value_result instead of
> make_errored_result and make_valued_result, as the former seem more
> grammatically correct.

I'll ponder that.

> The library does not provide a test/Jamfile, which means that as
> submitted it will not integrate into Boost's testing infrastructure.
> Given that a Jamfile that performs the equivalent of withmsvc.bat is
> trivial to write:
> 
>  import testing ;
> 
>  project : requirements <include>../include/boost/outcome/v1.0 ;
> 
>  run unittests.cpp ;
> 
> the omission is inexplicable.

The reason I omitted such a simple Jamfile is because it would be
non-representative of the proper test suite which is CTest based. The
withmsvc.bat file I placed there for reviewers to use in case they hate
cmake so much they can't follow the simple ctest instructions at
https://ned14.github.io/boost.outcome/introduction.html#unittests

> (Further integration into the Boost test
> matrix would ideally entail splitting unittests.cpp into separate .cpp
> tests so that success/failure can be tracked independently.)

You may not be aware that all test results can be individually written
to a JUnit XML file and thus fed to any test matrix. Indeed, the CTest
Dashboard at http://my.cdash.org/index.php?project=Boost.Outcome already
gives a test by test breakdown.

> Trying to test with VS 2017 fails with:

As reported very early on in this review with a special announcement,
Microsoft released VS2017.2 exactly at Outcome went to feature freeze. A
specially fixed version of the review edition for VS2017.2 was placed at
https://github.com/ned14/boost.outcome/tree/master_vs2017_fixed.

> It is not possible for a Boost release, say, boost_1_65_0.zip, to
> incorporate git submodules by reference, so there remains a question of
> what, exactly, is being submitted for inclusion. My attempts to clarify
> this with the author have proven unsuccessful.

It would be trivially easy for the Boost release process to bring in git
submodules recursively. There is no technical problem there, nor a legal
one if submodules use compatible licences which is on me to get right
(and I have). The sole obstacle is "that's not how things are done
around here", with no technical justification.

But I am surprised that you feel I did not clarify what is being
submitted for inclusion. I have stated on multiple occasions that if
submodules are a no-go, I would likely write a __has_include() to probe
for the submodule, and if not present then I would fall back onto Boost.
That way a single git repo can serve both as standalone and within Boost.

Indeed I have written out several implementation scenarios for that
situation, right down to the granular dependency file by dependency file
level. I am not sure what more detail I can provide at this time.

> I consider this a critical defect of the submission. It is simply not
> possible for a reviewer to give an opinion whether the submission ought
> to be included into Boost releases without it being known what,
> specifically, is being proposed to be included.
> 
> There is no problem with the library containing implementation details
> under its directory, but we need to know what these details are.
> 
> What the library actually needs as dependencies from boost-lite is not
> much and it's perfectly possible to extract the necessary parts and
> prepare a self-contained submission. This should have been done prior to
> the review.

It was not possible to know whether this library had any chance of
acceptance prior to submission, nor what attitudes would be to using git
submodules.

I had genuinely expected people to question API design decisions, not
get hung up on internal implementation libraries none of which interfere
with any external nor Boost code in any way. They are a pure
implementation detail, and in my opinion unimportant except regarding
their provenance i.e. is their author a well known expert in C++ and
highly likely to have written a high quality library?

(the answer is yes by the way, unless you feel my libraries to be
substandard. Martin's gsl-lite is great, but then he is Martin)

> The files in the library contain the following:
> 
> ```
> Licensed under the Apache License, Version 2.0 (the "License");
> you may not use this file except in compliance with the License.
> You may obtain a copy of the License in the accompanying file
> Licence.txt or at
> 
>    http://www.apache.org/licenses/LICENSE-2.0
> 
> Unless required by applicable law or agreed to in writing, software
> distributed under the License is distributed on an "AS IS" BASIS,
> WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> See the License for the specific language governing permissions and
> limitations under the License.
> 
> 
> Distributed under the Boost Software License, Version 1.0.
>    (See accompanying file Licence.txt or copy at
>          http://www.boost.org/LICENSE_1_0.txt)
> ```
> 
> This intends to dual license the library, but as written, it doesn't.
> What it does is apply the terms of BOTH licenses to distribution and
> use. This is also a critical defect. If the library is included in a
> Boost release, it should make it perfectly clear that it's licensed
> under the Boost license, and not under the intersection of Boost and
> Apache 2.0.

Would inserting this line between the stanzas suit you:

------------------------- OR ---------------------------------

If so that's no problem, the licence boilerplate is all generated by
script, it's done in a flash.

> Should Outcome be accepted?
> 
> In short, eventually. This, at present, is a NO.
> 
> Ideally, what I would like to see is the library resubmitted in a
> conventional form, consisting of exactly the files that are intended to
> go into the libs/outcome directory in boost_1_65_0.zip, licensed under
> the BSL, only containing the files necessary for the non-preprocessed
> version of the code to compile and work. In addition, the test/
> subdirectory would contain a Jamfile that is ready to integrate into the
> Boost test matrix.

This review seemed to me YES, but with the conditions you just listed.

Can you explain why you chose NO?

Niall

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