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-01-29 00:53:28

> - "View this code in Github"
>   This is fine for online viewing, but for a packaged release
>   it's better to link the local source.
> 
> - "contains an error information"
>   "information" shouldn't have an indefinite article.
> 
> PREREQUISITES:
> 
> - "It is worth turning on C++ 17 if you can, there are..."
>   comma splice.

All fixed in develop branch.

> BUILD AND INSTALL:
> 
> This section refers to all kinds of things that
> do not exist in the review version.  Such as:
> - CMake
> - Single header version
> - outcome/include/outcome.hpp

The documentation is for the standalone edition. If the library is
accepted, it will be adjusted for the Boost edition.

> DECISION MATRIX:
> 
> - The font size in the decision matrix scales with
>   the page size, which may make it unreadable.
>   (I had to zoom to 300%, before I could read the
>   third flow chart)  It's also completely unreadable
>   without javascript.

The decision matrix will likely be removed in the Boost edition, along
with all the other Javascript based stuff.

> TUTORIAL:
> 
> RESULT<T, EC>:
> 
> - OUTCOME_V2_NAMSPACE.
>   This seems singularly pointless.
>   How does accessing the namespace via a macro
>   help with backwards compatiblity?  It will
>   break source compatibility exactly as much
>   as if you just used a normal namespace.  If
>   you're concerned with binary compatibility,
>   then the only thing that matters is the
>   symbol mangling and there's no need to
>   expose this ugliness to the user.
>   Also, why is this attached to result, and
>   not in its own section.

The reason is that Outcome is expected to be used extensively in ABI
boundaries. So if DLL A has public APIs returning Outcome, and DLL B has
public APIs returning Outcome, but DLL A uses a different version of
Outcome to DLL B, we specifically want to force the compiler to utilise
the ValueOrError concept interoperation mechanism rather than having
incompatible Outcome implementations corrupt one another.

> - If some type `X` is both convertible to `T` and `EC`
>   parallel structure: 'convertible' should either be
>   placed before 'both' or be duplicated on both
>   sides of the 'and.'
> 
> - You say that it's ambiguous if the constructor
>   argument is convertible to both T and EC.  Is
>   it still ambiguous if it is exactly T or EC?
>   If so, that seems wrong.  If not, you need to
>   explain the behavior more clearly.

All implicit conversions disable if there is any relationship between T
and EC whatsoever. Yes, that's far too harsh. But that was what the
first review concluded was safe, and there was universal consensus on
that decision (eventually, it took a very long discussion).

I have fixed the tutorial wording to make this clear.

> - Why do you need to use tagged constructors
>   instead of just casting?  (I suppose the fact
>   that you felt the need to have these indicates
>   to me that an exact match /is/ ambiguous.)

The previous review wanted to avoid the surprises that one can
experience with constructing std::variant, hence the much stricter
restrictions.

> tutorial/result/inspecting:
> 
> - "Suppose we will be writing function..."
>   function needs an article.
> 
> - "integral number"
>   This is just a verbose way of saying "integer."
> 
> - "Type result<void>"
>   -> "The type result<void>"
> 
> - "Class template `result<>` is declared with attribute [[nodiscard]]
> which means compiler"
>   Add "the" before "class," "attribute," and "compiler."
> 
> - "In the implementation we will use function `convert`"
>   "*the* function"
> 
> - "#2. Function `.value()` extracts the successfully returned `BigInt`."
>   "*The* function".  Also it's an int, not a BigInt.

All fixed in develop.

> - I noticed that the starts of the list items after the
>   #N. are not vertically aligned, which looks a little odd
>   for #1, #2, #3, and #4, which are short enough to make it
>   obvious.  This appears to be because each item is
>   treated as a normal paragragh which happens to begin
>   with a number instead of a proper list item.

I'm personally fine with it.

> - "It implies that the function call in the second argument"
>   I presume that it takes a generic expression, not just
>   a function call.
> 
> - "It is defined as"
>   The antecedent of "It" is ambiguous.  It refers to
>   the function in the preceding sentence, but when
>   I first read it I assumed that it referred to "OUTCOME_TRY"

Fixed in develop.

> - I think the name OUTCOME_TRY is misleading because it has
>   the exact opposite meaning of "try."  try/catch stops
>   stack unwinding, but OUTCOME_TRY is used to implement
>   manual stack unwinding.

I think the ship has sailed on that. Swift, Rust etc all use try. The
proposal before WG21 is try. I think it better we conform to what other
languages are doing unless there is a good reason not to.

> - I'm not sure what I think of as_failure.  I'd actually
>   prefer to just write return r;

I think you mean `return r.error();` as the T types are not compatible.

> - "this will be covered later"
>   A link would be nice.

Improved in develop branch.

> - I can't see the point of having success<void>
>   default construct T.  It seems like dangerous
>   implicit behavior, unless you make it so that
>   success is variadic and constructs the result
>   in place. 

Can you explain why it would be dangerous?

> Also, there is no type named success.
>   It's called success_type.

Good point. The reason is that on C++ 17, we were using a success<T>
type directly constructible via template deduction guides. But the C++
17 standard is subtly broken here, I believe it gets fixed in C++ 20. So
I killed off the C++ 17 implementation, we now use the C++ 14
implementation everywhere.

I've logged the issue to https://github.com/ned14/outcome/issues/123

> tutorial/result/try
> 
> - OUTCOME_TRYV
>   I think it would be nicer if you used a single
>   macro that has different behavior depending
>   on whether it is passed 1 or 2 arguments. 

It'll need some preprocessor metaprogramming, but sure. Logged to
https://github.com/ned14/outcome/issues/124.

>
 It
>   would be even better if you could also merge
>   OUTCOME_TRYX in.  The behavior of OUTCOME_TRYV
>   is logically the same as that of OUTCOME_TRYX for
>   a result<void>.

Alas expression TRY is not portable across compilers, so I'll be keeping
that as its own macro to prevent portability problems.

> tutorial/outcome:
> 
> tutorial/outcome/inspecting:
> 
> - "outcome<> has also function"
>   -> "outcome<> also has a function"

Fixed in develop.

> - How does outcome::value interact with custom
>   error_code type?  It needs to know what exception
>   type to throw, no?

Covered in https://ned14.github.io/outcome/tutorial/default-actions/

> tutorial/payload:
> - "...symbolising that into human readable text at the
>   moment of conversion into human readable text"
>   This sentence does not parse.

Fixed.

> - "#4 Upon a namespace-localised `result` from library
>   A being copy/moved into a namespace-localised
>   `result` from C bindings..."
>   I can't understand this at all.  What is a
>   "namespace-localised result"  How does this
>   relate to a custom payload?  A plain error_code
>   has everything you need to set errno.

Namespace localised results are covered slightly later in the same
section starting from
https://ned14.github.io/outcome/tutorial/payload/copy_file2/.

Basically we define a local EC type to cause ADL for the extension
points into our local namespace, and then locally bind `result` into our
local namespace.

Most codebases using Outcome which are of any size will namespace
localise their result implementation in this fashion.

> general:
> 
> - In code examples, _'s can sometimes disappear
>   mysteriously.  I haven't figured out the exact
>   conditions, but resizing the window changes the
>   result.  Seems to be a result of overlapping with
>   the line underneath.

Ah, great spot. I had been stumped. Lines overlapping is a very likely
cause of that problem. Logged to https://github.com/ned14/outcome/issues/125

> - Also, the lines are too long.  They still wrap
>   even when I put the browser in full screen.
>   (Admittedly, the VM is only 1280x768)

Logged to https://github.com/ned14/outcome/issues/126

Thanks for the feedback!

Niall

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