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/