Re: [boost] [outcome] Review of Outcome

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 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/