Re: [boost] [review] Review of Outcome v2 (Fri-19-Jan to Sun-28-Jan, 2018)

Subject: Re: [boost] [review] Review of Outcome v2 (Fri-19-Jan to Sun-28-Jan, 2018)
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2018-02-04 01:41:33

> The design of the library is complex. I'm not sure that a simpler 
> design would be sufficient since it tries to do a lot of things. But 
> therein lies the rub: it smacks of trying to be too many things for 
> too many people, which requires it to be complex.

It really isn't complex, v2 is a marked simplification over v1 as
the previous review requested. It is overwhelming to the uninitiated
though, so I can see how it might appear to be complex. I would argue
that being overwhelmed with the possibilities would be a characteristic
of any vocabulary library. Outcome is not a niche purpose library like
we usually review here in recent years. Its scope of application has a
lot of knock on consequences. Most of the negative feedback posted in
this review so far is clearly grounded in concerns over effects on the
ecosystem, because if this thing enters Boost and it's broken, then all
the stuff built with it is also going to be broken. Such is the burden
of vocabulary libraries and language features.

> There are two principle templates, but potentially many different 
> types using them. There are many policies and function templates to 
> tweak the behavior of the library. The latter is somewhat disturbing 
> as one must always include all moving parts for a particular outcome 
> or result type in order to get all of the customizations. In 
> practice, everything is likely to be defined in a single header, so 
> that may not be a problem, but it seems complicated.

The policy classes seem to be much more scary than I had anticipated.
Honestly, try implementing your own. You'll be done within ten minutes.
They're very simple.

I agree that the granularity of the header includes needs to be improved
so people can include <boost/outcome/basic_result.hpp> and not drag
anything but basic_result. That's already logged, and will be done.

> What's more, some customizations are done using free functions, 
> found via ADL, and others via policy classes. That's discomfiting, at
> least.

The cause is the lack of C++ language support for something like
https://github.com/ldionne/dyno. A key goal of Outcome is
*non-intrusive* rule setting of how third party code ought to interact
without requiring modification of third party source code. It
unavoidably, given the limitations of the language, relies heavily on
ADL customisation points. I will say that where I was able to avoid ADL,
I did lift rule setting as much as possible into trait specialisation
in its own namespaces so it is clearly delineated and has minimum chance
of unexpected interaction.

I agree that what has resulted looks like a dog's breakfast, and again
it's overwhelming. I could have made everything ADL for purity, but I
find ADL worrying. Too much chance of unexpected surprise. Best
minimised to the absolute minimum in my book. But the consequence then
is that dog's breakfast. I'm not sure what else one can do, other than
ditch the non-intrusive interoperation support. And if reviewers did
want that gone, it could be removed and that would make things appear
much cleaner.

The complexity would then, of course, merely be pushed onto the end user
having to manually unpack and repack Expected into Outcome etc. Maybe
that's better. I had hoped for more feedback from reviewers on whether
it would be better or not.

> The docs are thorough, though in need of lots of editing. The 
> tutorial is very long and often overly complex. A more typical 
> approach is to use a tutorial as an introduction and to use a 
> separate section of the documentation for more advanced topics and 
> examples.
> 
> I generated a lot of comments while working my way through the 
> documentation. I've noted that some examples are poor choices for 
> justifying or illustrating the value of the library. I'll forward 
> them via email rather than include them here as they are extensive 
> (and I didn't even get through the entirety of the documentation).

I look forward to receiving those. But please be aware that I have
written a tutorial for Outcome five full times now. I am beginning to
realise that a tutorial which makes even a majority happy is not
possible given my very finite resources. I need to draw a line at some
point for my sanity, there needs to be a life which is not working
forever more writing Outcome documentation.

> That result<T,EC> is declared with [[nodiscard]] can be helpful. It 
> means the return value of a function returning a result cannot be 
> ignored. Unfortunately, the compiler doesn't require that the 
> programmer do anything with the result except save it. (A warning 
> might alert the programmer to do something with such a variable, but 
> there's no enforcement.) The same is not documented for outcome, 
> however. Thus, when using outcome, because one might need to convey 
> an exception to the caller, there is no compiler help to ensure the 
> programmer doesn't ignore the return value. Even with that fixed, 
> there's still no library help to ensure the programmer inspects the 
> return value.

The only solution to this that I am aware of is if outcome and result's
destructors throw. I felt that unwise, but it could be implemented if
reviewers felt it beneficial.

> I find that I cannot vote to accept the library into Boost. I am not 
> rejecting it, but I don't expect to make use of the library, so I 
> can't bring myself to vote for its inclusion in Boost. The library 
> seems sound, it seems like it could be useful, and it is reasonably 
> well documented. Inclusion in Boost could provide important exposure 
> that could shape the library before it is proposed for 
> standardization, which Niall intends, but I'm not convinced that's 
> reason enough to accept it into Boost.

Thank you for your review.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/