Re: [boost] [outcome] Andrzej's review

Subject: Re: [boost] [outcome] Andrzej's review
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2018-01-27 01:54:45

> How much effort did you put into your evaluation? A glance? A quick
>> reading? In-depth study?
> 
> I have read most of the final implementation. I have read many
> versions of the documentation. I have spent a lot of time (prior to
> the review) asking Niall lots of questions about the details. I have
> also built lots of small examples to test the library and some corner
> cases.

For the purposes of proper disclosure, I personally would consider
Andrzej a coauthor of v2 Outcome. He didn't write any code, but he was
my auditor throughout v2's development, watching new code land and
telling me all sorts of problems with it, usually through asking really
useful stupid questions which provoked me to think again. He upped my
game considerably, and I'd like to thank him once again for that, and
for all the new just submitted detailed bug reports. I shall attend to
them next week once this weekend's usual childcare is complete.

> Yes, conditionally. Here are three conditions:
> 
> 1. Documentation is adjusted to use boost-standard names of macros
> and namespaces. Also, it is not clear if Boost release process is
> capable of handling the technology that Niall has chosen. If not, the
> contents of documentation should be migrated to another format,
> manageable by Boost release process.

No problem. I have a script here already which converts the Markdown. If
accepted, I am happy to work with boost-website's maintainers to figure
out some formulation of tooling everyone is happy with.

> 2. Because there has been some confusion around the meaning of
> storing default-constructed `std::error_code` (is it an error or lack
> thereof?) and `std::exception_ptr` (e.g., see this issue: 
> https://github.com/ned14/outcome/issues/115), I request that you
> state in the docs, before the API Reference how library treats
> default-constructed values of `EC` and `EP`.

I am happy to spell out that the library makes no special interpretation
of `EC`'s or `EP`'s value at all.

For example, if `EC` matches `trait::has_error_code_v<EC>` in a
`result`, then `error_code_throw_as_system_error` policy is selected as
a default (https://ned14.github.io/outcome/tutorial/policies/builtin/).
If one then observes `.value()` when there is no value, the policy will
`OUTCOME_THROW_EXCEPTION(std::system_error(error))` or
`OUTCOME_THROW_EXCEPTION(std::system_error(make_error_code(error)))` as
appropriate.

I do not understand the differentiation of default construction over any
other construction. You can absolutely construct a `result<T>` using a
default constructed `error_code`, and upon `.value()`,
`OUTCOME_THROW_EXCEPTION(std::system_error(error_code()))` would occur.
Which would not be particularly useful, but then Outcome makes no
special interpretation of `EC`'s value at all. Just its type is to
activate traits (or not).

> For instance, you can say "default-constructed error_code is
> considered an error, and treated as such by the library", or "the
> library sometimes may make use of the fact that a
> default-constructed EC/EP is not an error, so make sure not to pass
> us such values": I am fine with any answer, but I need you to spell
> it out, so that I know what to expect.

Can you show me the exact page where I say the second sentence? Maybe
it's because it's 1:50 am and I'm too tired after a 6am start, but I
cannot find that sentence in the documentation.

> 3. Similarly, I request that you put in the docs what is the "value"
> of an object of type `outcome<>` or `result<>` in the sense in which
> John Lakos talks about "salient attributes". I know that T, EC, and
> EP constitute the value. But in the middle of the tutorial we learn
> that apart from these you also store "spare_storage" -- is it also
> included in copying and comparisons?

Yes in copying where copying is not explicitly done (i.e. type
conversion is performed).

No in comparisons.

I will add this to the documentation. Tracked at
https://github.com/ned14/outcome/issues/117

> Also, what does it mean that result is equal to
> optional. Define it somewhere and paste links to it.

Do you mean `optional<T>` being equal to `result<T, void>` or something?

Or do you mean via the `ValueOrNone` concept, or something else entirely?

> Finally, I wan to thank Niall for his now many-year effort to deliver
> this really cool library to the community.

Thank you for your review. You're right it's been a long road. Outcome
began life in 2014. I can't believe four years have passed on such a
small Result-ish type. But equally I don't think it could have been done
right any quicker: Outcome is not Expected because of the first Boost
peer review, that huge conversation just eight months ago led us to here
and now.

Niall

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