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

- "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:

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