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
I started my review, but I don't think I'm going to
get it finished, so I'm throwing in a partial review
- "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.
- "It is worth turning on C++ 17 if you can, there are..."
BUILD AND INSTALL:
This section refers to all kinds of things that
do not exist in the review version. Such as:
- Single header version
- 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
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.)
- "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`"
- "#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.
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
- "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?
- "...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.
- 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)
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk