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

Subject: Re: [boost] [review] Review of Outcome (starts Fri-19-May)
From: Paul A. Bristow (pbristow_at_[hidden])
Date: 2017-05-23 15:50:10

> -----Original Message-----
> From: Boost [mailto:boost-bounces_at_[hidden]] On Behalf Of charleyb123 . via Boost
> Sent: 19 May 2017 14:43
> To: boost_at_[hidden]
> Cc: charleyb123 .
> Subject: [boost] [review] Review of Outcome (starts Fri-19-May)
> 
> Hi Everyone,
> 
> The formal review of Niall Douglas' Outcome library starts today
> (Fri-19-May) and continues until Sun-28-May.
> 
> Your participation is encouraged, as the proposed library is uncoupled and
> focused, and reviewers don't need to be domain experts to appreciate the
> potential usefulness of the library and to propose improvements.  Everyone
> needs (and has suffered) error handling, and can compose an opinion on that
> topic.
> 
> Outcome is a header-only C++14 library providing expressive and type-safe
> ultra-lightweight error handling, suitable for low-latency code bases.

I see no problem at all in requiring C++14 (or C++17, or even C++20).  C++14  will soon be the default for LLVM and GCC.  I don't
see Outcome being retro-fitted to those stuck with older compilers.  If a project is being retrofitted, then it is also a good time
to upgrade to the latest compiler.
 
> Key features:
> 
> *- Makes using std::error_code from C++11's <system_error> more convenient
> and safe
> *- Provides high-quality implementation of proposed std::expected<T,E> (on
> C++20 standards track)
> *- Good focus on low-latency (with tests and benchmarks)
> *- Error-handling algorithmic composition with-or-without C++ exceptions
> enabled
> *- No dependencies (not even on Boost)
> 
> This review is timely, as C++17 brings us std::optional<T>.  The upcoming
> std::expected<T,E> (an implementation provided in Outcome) is a
> generalization of std::optional<T> that provides a <success|fail> value,
> where the unhappy result is a 'std::error_code' or an instance of
> "your-chosen-error-type".
> 
> The library further provides 'outcome<T,error-code,exception-ptr>' for
> handling <success|error|exception> to safely wrap throwing APIs.
> 
> Documentation:
>   https://ned14.github.io/boost.outcome/index.html
> 
> ACCU 2017 talk including design rationale:
>   https://www.youtube.com/watch?v=XVofgKH-uu4
> 
> GitHub:
>   https://github.com/ned14/boost.outcome
> 
> Latest tarball:
>   https://github.com/ned14/boost.outcome/releases/download/boost_peer_review3/boost.outcome-v1.0-source-201705111650.tar.xz
> 
> Note:  Tarball might be easiest; but if you want to clone from GitHub
> directly, after the clone you should run the following command to get the
> source zip exactly:  git submodule update --init --recursive

> Please post your comments and review to the boost mailing list
> (preferably), or privately to the Review Manager (to me ;-). Here are some
> questions you might want to answer in your review:
> 
> - What is your evaluation of the design?

Terrifying.  But making things simple and efficient for the user usually requires that.
 
> - What is your evaluation of the implementation?

Way above my pay grade.  Complex for reasons I don't pretend understand.  I'd hope that any actual released stuff would be somewhat
simpler.  I understand that the requirement for as-yet-unfinished standards and compilers makes life very difficult -
chicken'n'eggy.

It is clear that a massive amount of work has gone into this, including a lot of worthwhile worrying about efficiency. So I think it
much be considered quite refined. (It might still be wrong, but I have faith).

It doesn't fit well into the Boost framework, but perhaps that is intentional?
 Having an include outcome.hpp that only consists of macros is a bit alarming.   

> - What is your evaluation of the documentation?

Hard going.  Niall knows far, far, far too much already?  Get someone else to write a tutorial on 'how to use outcome'?

> - What is your evaluation of the potential usefulness of the library?

Very useful and very important.
 
> - Did you try to use the library? With what compiler? Visual Studio Pro 2015 update 3. 

Yes, via git to a Boost develop tree.

I did 

modular-boost/libs> git clone https://github.com/ned14/boost.outcome.git
modular-boost/libs> git submodule update --init --recursive

I:\modular-boost\libs>git clone https://github.com/ned14/boost.outcome.git
Cloning into 'boost.outcome'...
remote: Counting objects: 7692, done.
remote: Compressing objects: 100% (145/145), done.
remote: Total 7692 (delta 179), reused 229 (delta 139), pack-reused 7397
Receiving objects: 100% (7692/7692), 3.49 MiB | 102.00 KiB/s, done.
Resolving deltas: 100% (4929/4929), done.
Checking connectivity... done.

(I expected a folder in /libs called outcome, but I got one called boost.outcome so it wasn't in the alphabetical order or format
that I expected, but I found it :-)

I:\modular-boost\libs>cd ./boost.outcome

I:\modular-boost\libs\boost.outcome>git submodule update --init --recursive
Submodule 'doc/html' (https://github.com/ned14/boost.outcome.git) registered for path 'doc/html'
Submodule 'include/boost/outcome/boost-lite' (https://github.com/ned14/boost-lite.git) registered for path
'include/boost/outcome/boost-lite'
Cloning into 'I:/modular-boost/libs/boost.outcome/doc/html'...
Cloning into 'I:/modular-boost/libs/boost.outcome/include/boost/outcome/boost-lite'...
Submodule path 'doc/html': checked out 'caf37a25ea541ca2c8309df17329bdb23037e7fd'
Submodule path 'include/boost/outcome/boost-lite': checked out 'ed8678b7b2c17f1064301e3f713a6593fe91f7f1'
Submodule 'include/gsl-lite' (https://github.com/martinmoene/gsl-lite.git) registered for path
'include/boost/outcome/boost-lite/include/gsl-lite'
Submodule 'pcpp' (https://github.com/ned14/pcpp.git) registered for path 'include/boost/outcome/boost-lite/pcpp'
Cloning into 'I:/modular-boost/libs/boost.outcome/include/boost/outcome/boost-lite/include/gsl-lite'...
Cloning into 'I:/modular-boost/libs/boost.outcome/include/boost/outcome/boost-lite/pcpp'...
Submodule path 'include/boost/outcome/boost-lite/include/gsl-lite': checked out 'e6fb512f372d58a8f11dd99f74e79a3c9f0a9d15'
Submodule path 'include/boost/outcome/boost-lite/pcpp': checked out '488dcd2ca0588bac267cfc32b8ec17c2620c4d5a'

and I could then read the documentation at

modular-boost/libs/boost.outcome/doc/html/index.html

I expected to be able to run the examples using b2 but sadly this is not supported.  Is there any reason why not (apart from an
aversion to bjam?)  Boost has a system (even if not the best or Niall's preference) for running on multiple platforms and this
doesn't fit with it which feels Boost-unfriendly at least.

So I fired up MSVC  IDE and added a new project and added the simple_example.cpp and built and ran it in my normal way.  No need for
adding modular-boost to the search list.  (But there is an worrying load of Boost-lite stuff hanging on which did not feel good.  

1>------ Rebuild All started: Project: outcome, Configuration: Debug x64 ------
1>  simple_example.cpp
1>  outcome.vcxproj -> J:\Cpp\Misc\x64\Debug\outcome.exe
1>  outcome.vcxproj -> J:\Cpp\Misc\x64\Debug\outcome.pdb (Full PDB)
1>C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(203,5): error MSB3073: The command
""J:\Cpp\Misc\x64\Debug\outcome.exe" --build_info=yes
1>C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(203,5): error MSB3073: :VCEnd" exited with code
20.
========== Rebuild All: 0 succeeded, 1 failed, 0 skipped ==========

I noted the lack of a licence and copyright statement throughout.

However, it appears to run OK from the IDE 'start without debugging' but produced no output.  Not comforting.  It really isn't my
idea of a simple example.  So I tried expected_payload1.cpp since it looks (and was recommended) as a better working example. (VS
moans about deprecated Posix names, but hey...  and boost.outcome\example\expected_payload2.cpp(27): warning C4267: 'argument':
conversion from 'size_t' to 'unsigned int', possible loss of data is ugly).

I'm cool with namespace versioning, but I would hope it was a temporary bit of clutter/complexity.

I also tried expected_payload2.cpp and that was more informative.  I'd put this example first.

The output "Failed to open file somefile due to 2" isn't an exemplary error message, even if it is only trying to show how outcome
works.  (Putting this string "Failed to open file somefile due to 2" as a comment would make it easy for the reader to understand
without actually running the example).

I tried to run the test expected_pass.cpp but got

modular-boost\libs\boost.outcome\test\expected_pass.cpp(15): fatal error C1083: Cannot open include file:
'../boost-lite/include/boost/test/unit_test.hpp': No such file or directory

but it looks plausible.  Again I'd expect a jamfile to do this.

> - How much effort did you put into your evaluation? 

Re-reading documentation and another quick skirmish. 
 
> - Are you knowledgeable about the problem domain?

No.
 
> And most importantly:
> 
> - Do you think the library should be accepted as a Boost library?

I dream of a world where error codes had never been conceived and there was only one way of doing things where exceptions worked
efficiently using hardware better.
(I have seen convincing evidence that the cost of exceptions is over-estimated).

I always wake up and curse the extra work that any error handling causes (usually 2 * pi *
however-long-it-took-to-write-the-code-to-start-with).

I found Outcome hard going, but I fear that is because it *is* hard.

As Donald E. Knuth warned "Software is HARD".

It is certainly a major weakness of much software that the result of an error is pretty much "Sorry - something went wrong :-( ".
At best one gets "File not found." - but no clue t which F*&^ing file, or "trying to write outside array bounds", but not what array
with what bounds, nor to which element,
so Niall is right to put delivering 'payload' as a major goal.

The proof of the pudding is in the eating.  So far there are few tasters.

For acceptance it really must have overall boostification to fit into Boost conventions, where possible or appropriate.  I am
sympathetic to authors reluctance to do this until acceptance is assured.

In the end, we now have to decide if this is all going to be 'OK in the end' and have faith in Niall's skill, judgement, and
determination to see this functional and finished.  I do.   So that's a yes.

Paul

---
Paul A. Bristow
Prizet Farmhouse
Kendal UK LA8 8AB
[phone]
Nits noted
==========
Lots of files missing Boost licence.  This is of course essential for Boost.
Some punctuation and a nicer:
This eliminates the fragile switch statements converting between error code domains in favour of an information loss-free
transmission. The cost is once again a loss of type safety because if a function might return an error code, it should never be able
to return and the compiler will not complain.