Re: [boost] [outcome] Review of Outcome

Subject: Re: [boost] [outcome] Review of Outcome
From: Peter Dimov (lists_at_[hidden])
Date: 2017-05-29 03:53:37

Niall Douglas wrote:
> > 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.

I don't object to expected<> being included; I just don't think that it has 
to have the spotlight. You're designing result<> and outcome<>, not 
expected<>, where you're tracking Vicente's papers. result/outcome represent 
a different paradigm.

> 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 don't agree here. One or both of result<T>/outcome<T> are a perfect fit 
for <system_error>. They expand the potential utility of std::error_code 
enormously and allow previously dual interfaces (f.ex. filesystem) to be 
expressed much more cleanly.

Nothing against expected<>, I hope that we get it right as well. Horses for 
courses.

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

It's not as useless as you might think.

Chaining results occurs during stack "unwinding" due to an error in a low 
level API. It returns a error result<>, the upper layer also returns an 
error, and so on upwards the stack, with some layers however deciding to 
replace the result<> with their own instead (as you do in your example.)

At the uppermost level, you get an error result<> and it'd be extremely 
valuable if when logging the error you could follow the chain and log all 
intermediate results.

For that, a ring buffer with 2048 entries (or however many you had) is 
plenty enough; there won't be 2048 failed operations in-between (well, there 
might be because threads, but you take what they give you; it can't get any 
worse than not having the information at all.)

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

The ring buffer hasn't gotten much review love. I like the idea. Although, 
as I mentioned, I'd have separated it slightly more from the rest so that it 
can be excised or reworked if necessary without touching the main API of 
result/outcome.

> > (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.

Yes, I looked at boost-lite's testing header and saw the JUnit XML support. 
But there are two reasons to split the test into separate files. One, if one 
of them fails to compile, you still get results in the matrix for the rest; 
and two, the Boost testing infrastructure doesn't do JUnit XML and is not 
expected (I think) to acquire such an ability in the near future. How it 
works is you list the tests in you Jamfile and the testing script then 
parses the b2 output and uploads the results. Here, for example:

http://www.boost.org/development/tests/develop/developer/smart_ptr.html

This is the Jamfile:

https://github.com/boostorg/smart_ptr/blob/develop/test/Jamfile.v2

The test-suite and the [] can even be removed,

    run smart_ptr_test.cpp ;
    run weak_ptr_test.cpp ;

and so on is all that's required.

> It would be trivially easy for the Boost release process to bring in git 
> submodules recursively. There is no technical problem there, ...

Boost releases do not contain foreign code. Everything in the release comes 
from boostorg repos, all of which are licensed under BSL.

This is not just some sort of a whim. We've been building our reputation for 
years and Boost releases carry an implicit unwritten guarantee that we've 
vetted the code there both technically and legally. This was important for 
corporate adoption - there is a reason why when unspecified multinationals 
allow any outside code, it's most often Boost - and we carry a certain 
obligation to continue in the same manner or risk losing the trust we've 
gained.

Incorporating foreign code from non-boostorg repos is a significant policy 
change. Steering committee + release managers significant. If you want to 
place your eggs in that basket, fine; I however don't think that a library 
review is the proper channel through which one effects such a policy change.

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

What you can provide is, quite simply, the proposed contents of the 
directory libs/outcome as it should appear in the next Boost release.

> Would inserting this line between the stanzas suit you:
>
> ------------------------- OR ---------------------------------

Not really. There needs to be a prominent header before the two licenses 
that states unequivocally that the library is dual-licensed. Something like 
(but I'm not a lawyer)

"Licensed under the Apache License, Version 2.0, or alternatively under the 
Boost Software License, Version 1.0. You may use the library according to 
the terms of either of those licenses, at your option."

Even so, dual licensing is still an unnecessary obstacle you're erecting 
before your library. Just use the BSL and be done with it. I don't 
understand you love of setting precedents. This, too, is steering committee 
material, as everything else in Boost is BSL.