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: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-01-28 19:11:52

AMDG

I started my review, but I don't think I'm going to
get it finished, so I'm throwing in a partial review
as is:

HOME:

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

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

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.

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.

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

- 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.)

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.

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

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

- 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'm not sure what I think of as_failure.  I'd actually
  prefer to just write return r;

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

- 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.  Also, there is no type named success.
  It's called success_type.

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

tutorial/outcome:

tutorial/outcome/inspecting:

- "outcome<> has also function"
  -> "outcome<> also has a function"

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

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

- "#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.

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.

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

In Christ,
Steven Watanabe