Re: [boost] [review] Review of Outcome (starts Fri-19-May)

Subject: Re: [boost] [review] Review of Outcome (starts Fri-19-May)
From: Robert Ramey (ramey_at_[hidden])
Date: 2017-05-27 17:56:06

On 5/27/17 6:29 AM, Niall Douglas via Boost wrote:
> On 27/05/2017 01:22, Robert Ramey via Boost wrote:

>>> *- Error-handling algorithmic composition with-or-without C++ exceptions
>>> enabled
>>
>> I would drop just about all the references to exceptions.  It's an
>> orthogonal issue.  If you've decided you need exceptions or that they
>> are convenient in your context, you're not going to change to outcome.
>> If you've already decided on returning error indicators (for whatever
>> reason), you're not going to switch to exceptions.  You might want to
>> switch to outcome.  So comparison with exceptions isn't really relevant
>> at all.
> 
> You may have a missed a major use case for outcome: calling STL code in
> a C++ project which is C++ exceptions disabled. 
These applications are not using exceptions right now.  They might use 
outcome as an alternative to an integer return code.  But exceptions are 
not relevant to this case.

Much of the games and
> finance industry are interested in Outcome precisely because of the ease
> Outcome enables passing through C++ exceptions thrown in a STL using
> island through code with exceptions disabled.

I have to confess it never occurred to me what happens when a program 
compiled with exceptions disabled uses the "throw" statement. I would 
have assumed that it would fail to compile.

> Even if you're not doing that, another major use case is to keep
> exception throws, and having to deal with control flow inverting
> unexpectedly, isolated into manageable islands. The rest of your
> intermediate code can then assume no exception throws, not ever. That
> means no precautionary try...catch, no need to employ smart pointers, no
> need to utilise RAII to ensure exception safety. It can be a big time
> saver, especially during testing for correctness because it eliminates a
> whole load of complexity.

I get this.  But this is likely already being addressed with code like:

// presume that sqrt throws when passed a negative number or Nan or???
float safe_sqrt(float x, int & error_flag){
        float y;
        try {
                y = sqrt(x);
                error_flag = 0;
        }
        catch(const std::domain_error & de){
                error_flag = 1;
        }
        return y;
}

which you would recommend replacing with something like:

// presume that sqrt throws when passed a negative number or Nan or???
outcome<float, int>
safe_sqrt(float x){
        float y;
        try {
                y = sqrt(x);
                return outcome(y);
        }
        catch(const std::domain_error & de){
                return outcome(0);
        }
}

Which I would agree would be an improvement.  BUT it really has nothing 
to do with exceptions per se. outcome is replacing exceptions, it's 
replacing an ad hoc integer error code/result combination with a better one.

So I would personeally have limited references to exceptions to a small 
example similar to the above.

>> Pretty much the same for error codes.  You might or might not use them -
>> but this is a totally orthogonal question as to whether you use outcome
>> or not as it is (I think) designed to handle any type as an error return
>> type
> You are thinking of expected<T, E> where E is selectable by the end
> user. 

I wasn't thinking of Expected.  I saw Vicent's presentation of it some 
years ago and found it unconvincing so I forgot about it.

> Outcome's outcome<T> and result<T> hard code the error type to
> error_code_extended. So error codes are the only game in town for those
> refinements.
Hmmm - it didn't get that the E parameter had to be error_code.  I 
thought it could be anything - which would have decoupled outcome from 
error_code.

>> Eliminating all the irrelevant material would make the package much,
>> much easier to evaluate and use.
> 
> With respect Robert, I don't think you understood all the use cases.

Right - that's my complaint.  I read the documentation.  I saw a 3 cases 
in the introduction.  It wasn't apparent that it would be useful for 
things other than that.

> Most have called for significantly *more* documentation to cover in
> detail even more use cases, not less.

Maybe - Either there other use cases that aren't in the docs or they're 
wrong. If they're other uses cases which aren't obvious, that's sort of 
red flag for me.  It suggests that either the library might have 
non-obvious behavior.  But I'm just commenting on what I currently see.

> (Incidentally I tried less documentation three times previously. For
> some reason, people need to be hand held more when it comes to
> describing really simple and obvious-to-me at least stuff)

More is not necessarily better

>>> *- No dependencies (not even on Boost)
>>
>> Yowwww. This terrible.  I looked a little through boost-lite.
>> config.hpp - repeats a lot of stuff from boost/config.  Doesn't seem to
>> provide for compilers other than gcc, clang and some flavors of msvc.
> 
> It actually provides for all the C++ 14 compilers currently available.
> If they are not MSVC, GCC nor clang, they pretend to be one of those, so
> the same detection routines work.

> You may also have missed the extensive use of C++ 17 feature test
> macros. All C++ 14 compilers, even MSVC, support those now.

Right - this is the wrong approach.  If you think that boost/config.hpp 
needs fixing you can address that.  But mixing configuration into a 
particular library just generates more work rather than factoring out 
into the best implementations.

>> Looks like some stuff might have some value - but with not docs/comments
>> it's pretty much useless if something goes wrong - like a compiler upgrade.
> 
> The C++ 17 feature test macros ought to be observed by all future
> compiler upgrades.
> 
>> chrono.hpp - what is this doing in here?
>>
>> precompiling <tuple> etc.....   Looks like you're trying to solve some
>> other problem here -
>>
>> goes on an on.

> I think you didn't realise those are the namespace bind files allowing
> the C++ 11 STL to be switched with the Boost STL. They are auto
> generated by a clang AST parser.

gratuitious complexity.

>> The questions of pre-compiled headers/modules/C++11 subset of boost have
>> nothing to do with outcome and shouldn't be included here.  Focus on one
>> and only one thing: The boost.outcome library.  It's quite a small
>> library and trying to make it carry the freight for all these other
>> ideas will deprive it of the value it might have.

I'd like to.  But it looks like accepting outcome into boost will 
effectively mean accepting a boat load of other unrelated stuff.  So if 
you want to address just outcome, remove all the other stuff.

> That would not be a widely held opinion. Most end users of Outcome don't
> want any Boost connection at all. 

No problem.  If you believe that don't submit it to Boost.

> They do want excellent top tier cmake

LOL - which would require CMake being quality product - which I don't 
believe it is.

> support, with automatic usage of compiler technologies such as
> precompiled headers, 
These are available to users - not required to add to libraries
C++ Modules,
Not ready yet
clang-tidy passes,
don't know what that is.
unit test
All libraries support unit test without adding confusing stuff to their 
library

> dashboarding, 
not related to the library
sanitisers,
already supported.
ABI guarantees,
This is interesting and I might accept this.  But but I'm not crazy 
about it being snuck in the back door.
and lots of other goodies.
nope

> boost-lite provides lots of common cmake machinery used by all my
> libraries to save me reinventing the wheel for each.
LOL - that's not what it looks like. I don't think library code should 
be tied to anything outside it.

> Again, none of this matters to an end user who just wants to #include
> and go. You can do exactly this with Outcome. It neither uses nor
> interferes with anything in Boost. It is an excellent neighbour,
> including to other versions of itself.
> 
> Just because you don't understand the value of exception_ptr and nested
> exceptions doesn't mean you are in a majority. Nested exceptions support
> in the C++ 11 STL wouldn't be there unless WG21 thought them highly useful.

Right - I'm just relating the experience of one user - myself. But I 
don't think I'm untypical. I have to say I've never seen exception_ptr 
or nested_exception in any code. If you think this is important, you 
need to provide information on or at least point to references which 
describe how this stuff because most of us who don't read the standard 
text don't even knows it exists.

>> I expect to see type requirements (aka concepts) for T and E.  Can any
>> types be substituted?  It's a mystery.  Hmmm maybe initial example
>> and/or looking at the source code will be useful.  First of all, the
>> example has no #include <output.hpp> or some such.  I go looking for
>> that and ....  I can't find it.  It's somewhere hidden through a maze of
>> ... preprocessing?
> 
> There are static asserts which fire if the types used don't meet their
> requirements.

LOL - so one doesn't know what the requirements are until you compile 
the code?  How can write code this way?

> You are correct they are not documented in the reference API docs. There
> is an open issue to fix that.

OK - but

> 
>>> - What is your evaluation of the implementation?
>>
>> Needs to be slimmed down and all the extraneous stuff removed.  This
>> isn't a vehicle for upending boost.  This is trying to get as modest
>> simple header only library into boost in a way which makes it useful to
>> users nothing more.
> 
> As I have said many times now, end users can #include and go. 

That's certainly not apparent from the examples.

> They don't care, 

LOL - this user cares

nor need to care,

I'm not convinced of this.

> how the implementation works so long as it cannot cause them misoperation.

Up to a point this is true.  But for a simple type such as this one I 
have some expectation what the implemenation should look like and when I 
look here - I can't see anything that makes much sense to me.

>> If one thinks I'm being two harsh, I propose the following experiment:
>> Take a pair of left over interns from C++Now and direct them to the
>> outcome git hub package.  Give them a simple task to use the outcome
>> library.  See if they can complete it in one hour or less.
> 
> I have seen completely uninitiated users get up and running with Outcome
> in less than five minutes. 

Honestly I can't believe this.   I would have to see it for myself.

> If the end user has programmed in Rust or
> Swift before, they just need to be told:
> 
> #include "boost.outcome/include/boost/outcome.hpp"
> 
> boost::outcome::expected<T, E>
> 
> ... and they're good to go.

LOL - I'll start learning Rust so I can use outcome.
> 
>> On the other hand, I believe it wouldn't be too tough to take the pieces
>> of this library already made and compose them into a more tightly
>> focused package that would fit into boost in a natural way that users
>> would useful and easy to use.  But the look and feel of the package
>> would be entirely different than the current one so It would really have
>> to be reviewed again.  I also believe that if the package were
>> re-formulated as I suggest, the review process would be much, much less
>> onerous for everyone involved.
> 
> Thanks for the review.

You're welcome

Robert Ramey
> 
> Niall
> 
>