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

27 May 2017

      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
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 where E is selectable by the end
user.
...
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
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.

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
...
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
LOL - which would require CMake being quality product - which I don't 
believe it is.

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