Re: [Boost-users] [boost] Boost.Outcome review - First questions

Le 21/05/2017 à 15:59, Niall Douglas via Boost a écrit :

...

A minor issue

* "As inferred in the example code cases earlier, you can check if an |expected| has an expected value via a simple test for boolean true or more explicitly via |.has_value()| before retrieving the value using |.value()|."

This is not correct. It is correct for the de-reference operator, but .value() don't needs any check with .has_value(). If you don't check before using .value(), it may throw bad_expected_access<E>. So you have to fix the text.

...

Instead of some deviation, I would expect a report of all deviations and a rationale explaining why it is better to do this way. I believe the list of deviations is complete, apart from the defects you found listed below which I will fix. Explaining why I deviate in detail isn't appropriate for the Outcome docs, I have raised why with you privately at length. Depending on what is decided at Toronto, I may write up remaining concerns into a WG21 paper, but in the end Outcome's Expected will track yours more closely than it has, so whatever you decide I'll do too. My idea is to forward your concerns to Toronto, but for that I need arguments that I expected to find in this section.

...

# "Types |T| and |E| cannot be constructible into one another. This is a fundamental design choice in basic_monad to significantly reduce compile times so it won't be fixed."

* "Note though that |expected| is permitted (which is legal in the LEWG proposal)."

What are the compile time cost |associated the std::experimental::expected? Outcome doesn't do SFINAE on its main constructors. It relies on simple overloading which is much lower compile time cost. Maybe it has lower compile time, but it is not correct, isn't it? Would you suggest that std::expect shouldn't do SFINAE?

...

I don't see what |expected could mean. It's legal in the LEWG Expected proposal. It's essentially a boolean. Hugh!

...

# |unexpected_type<E>| is implemented as an |expected| and it lets the basic_monad machinery do the implicit conversion to some |expected| through the less representative to more representative conversion rules. Note that our Expected passes the LEWG Expected test suite just fine, so you probably won't notice this implementation detail in your code.

The test of my POC implementation couldn't be complete, so this is not a sign of compatibility.

Which operation allow to convert expected to expected and what is the expected behavior if expected has a value? basic_monad, separate to Expected, implements implicit conversion from any less representative form to any more representative form. So: Could you point me to the documentation?

result<void> => result<T> result<T> => outcome<T> expected => expected

... and so on.

That's why implementing unexpected_type as an alias of expected "just works".

A current outstanding bug (https://github.com/ned14/boost.outcome/issues/16) is failure to compile:

result<void> => result<T>

... when T has no default constructor because a valued void state turns into a default constructed T state. That *should* fail to compile of course, but the as_void() free function also fails to compile that, and it should work because otherwise the BOOST_OUTCOME_TRY() machinery fails to compile when the T of the enclosing function doesn't have a default constructor. It's my next issue to fix, it'll be done soon. But what is the meaning if the result<T> if result<void> has a value?

...

# Our |make_expected()| and |make_unexpected()| functions deviate significantly as we believe the LEWG ones to be defective in various ways. This topic will be discussed in Toronto and reconciliation later is highly likely.

Please, could you tell how those are defective so that we can fix them? Otherwise I don't see what we could discuss in Toronto. The most important problem by far is the inability to do make_expected<const Foo>(Foo()) => expected<const Foo>. I suppose const volatile also should be supported. I have already raised this with you, you said it will be discussed at Toronto. Yes, it will. This is not a good reason to don't provide what already is in or what we discussed together.

Whatever you end up doing to make_expected() etc to enable const T and const E, I will replicate in Outcome's Expected. Until then I deviate to allow those. expected is useful sometimes. How you deviation allows it in a better way?

...

# Our |value_or()| member functions pass through the reference rather than returning by value (we don't understand why the LEWG proposal does except that |std::optional<T>| does, which itself is a bad design choice and it should be changed).

Well, what is the advantage to returning by reference? You don't cause .value_or() to surprisingly fail to compile when T lacks a move or copy constructor. The reference returning edition does not impose the requirement for T to have a move or copy constructor. This choice for .value_or() interferes less on end users. Do you have cases where expected has a T that is not move/copy constructible? How can you return it? The fact optional implemented .value_or() with a return by value is a mistake. Expected need not repeat that mistake. Could you point me to the C++ standard std::optional defect? Have you created one?

...

# Our Expected always defines the default, copy and move constructors even if the the type configured is not capable of it. That means |std::is_copy_constructible| etc returns true when they should return false. The reason why is again to significantly improve compile times by hugely reducing the number of templates which need to be instantiated during routine basic_monad usage, and again this won't be fixed. Instead use the static constexpr bools at:

Defining these operation when they can not be implemented means that you cannot use SFINAE on this type. Maybe I'm wrong, but I believe we want to check if a type is constructible, ... using the C++ standard type traits. Correct. static constexpr bools provide the information instead. If reviewers like it, we may inject correct answers into namespace std for the type traits (this is permitted by the C++ standard). Could you elaborate?

...

Next follow the first questions I have mainly from the reference documentation of expected.

In general I find most of the operation not specified enough. I would like to have something in line with the standard way of specifying an interface. Already logged to https://github.com/ned14/boost.outcome/issues/26

...

* it is not clear from the reference documentation if expected is valid? Do you mean there is no reference in the API docs to an expected specialisation? I didn't add one as I figured it was self evident and unsurprising. Also, if we document expected, we'd have to also do result<void>, outcome<void>, expected etc and to be honest, they all behave exactly as you'd expect a void typed edition would: wherever there is a T, you get T=void. Nothing special. Maybe or maybe not. What is the signature of value_or?

...

|* |is expected the sum type of T, E and empty? and exception_ptr? expected may only have the state of T or E. The valueless by exception state has been eliminated recently in develop branch. Is this the case also for outcome/result/option?

...

|"outcome<T>| can be empty, or hold an instance of a type |T|, or hold an instance of |error_code_extended|, or hold an instance of |std::exception_ptr|. This saves you having to wrap an error code into an exception pointer instance, thus also avoiding a memory allocation."

std::experimental::expected requires that its Error type to be no throw constructible, copyable and movable. Those static asserts are gone from develop branch as we now fully implement the never empty guarantee for Expected. Is there

...

What are the exception warranties for |error_code_extended? It never throws, not ever.

This is pretty clear in the reference docs. Everything is marked noexcept. I don't see it in https://ned14.github.io/boost.outcome/classboost_1_1outcome_1_1v1__xxx_1_1er... Could you point me where noexcept is?

...

What are the exception warranties for |outcome<T>||? The tutorial covers those. The reference docs will too when issue #26 is implemented. The noexcept modifiers on most of the member functions are pretty clear however. This is the kind of information we want on the reference documentation. Could you tell us what the issue #26 The default actions for each of .value(), .error(), .exception() etc need to be spelled out in the reference docs https://github.com/ned14/boost.outcome/issues/26 will add to the docs and how this issue is related? What about the other functions, constructors, assignments ...

...

* inplace construction

What is the difference between:

constexpr expected (value_t _) noexcept(std::is_nothrow_default_constructible< value_type >::value) Implicit constructor of a valued monad (default constructed)

and

constexpr expected (in_place_t, Args &&... args) noexcept(std::is_nothrow_constructible< value_type, Arg, Args... >::value)

when there is no Args. IMO the second subsumes the first and so there is no need fro the first overload and the value_t tag. The former has lower compile time load for valued default constructing an instance, and it's the constructor used internally by helper functions etc. Maybe this should be private and these helper functions friend. Id expected is default constructible, why do we need it? What is the difference between

constexpr expected () and constexpr expected (value_t _) ?

The latter is there for emplacement construction for those willing to pay the compile time cost.

...

* default constructor

constexpr expected () noexcept(is_nothrow_default_constructible) Default constructor.

This doesn't say what the default constructor does. As per LEWG Expected, T is default constructed. The reference docs will be fixed with issue #26. I insists #26 has nothing to do with constructors and I'm (we're) interested in knowing how this will be fixed.

...

* Shouldn't

constexpr expected (in_place_t, std::initializer_list< U > l) noexcept(std::is_nothrow_constructible< value_type, std::initializer_list< U >>::value) Implicit constructor from an initializer list.

be

constexpr explicit expected(in_place_t, initializer_list<U> il, Args&&... args); Ah yes I had forgotten about that.

You are correct, but I left it unfixed to remind me to ask here what would be a suitable unit test for testing an "initializer_list<U> il, Args&&... args" constructor where the Args&&... gets used correctly. I was unsure of the semantics. Take a look at std::optional test if you are curious.

Also, it's logged to https://github.com/ned14/boost.outcome/issues/27.

...

* What is void_rebound and where it is defined It was accidentally omitted from the reference docs. It is "using void_rebound = rebind<void>". Logged to https://github.com/ned14/boost.outcome/issues/28.

...

* construction from error type

Why do you prefer an implicit conversion. This deviation must be in the rationale of the differences. The deviations list already explains that types T and E cannot be convertible into one another because we use simple overloading to avoid using SFINAE to keep compile time load low.

Code written for LEWG Expected will, unless types T and E are convertible into one another, work as expected. You don't answer to my question. Why implicit? What is wrong with the constructor

expected(unexpect_t, error) ?

...

* raw types

It is not clear what is the difference between xxx and raw_xxx Logged to https://github.com/ned14/boost.outcome/issues/29

Thanks for creating the issue, but I'm interested in knowing it now during the review. Otherwise I couldn't accept even conditionally the library.

...

* what is the sizeof expected ? It says already at the top of the page. It's max(24, sizeof(R)+8) on 64 bit CPUs.

An now that you will ensure the never empty warranties?

...

* What is expected<Policy> in Looks like a misparse by doxygen. Such a thing doesn't exist. Logged to https://github.com/ned14/boost.outcome/issues/30

...

* bool conversion should be explicit Another doxygen bug. Logged to https://github.com/ned14/boost.outcome/issues/31.

...

* is_ready has nothing to be with expected. Already removed from develop branch.

...

* Shouldn't value_or return by value? otherwise what would be the difference between get_or and value_or? The get_*() family of functions is scheduled to be removed as per peer review feedback by https://github.com/ned14/boost.outcome/issues/24

Okay.

...

* emplace. Missing overload

template void emplace(initializer_list<U>, Args&&...); Great catch! Logged to https://github.com/ned14/boost.outcome/issues/32

...

* Missing preconditions on the observers, which make them wide operations. Saying that they return an address makes them wide operations however accessing to the contents of this address will be undefined behavior.

I prefer the standard way, defining a narrow function and stating clearly the pre-conditions. Issue #26 will fix these.

...

* what error_or https://ned14.github.io/boost.outcome/structboost_1_1outcome_1_1v1__xxx_1_1p... returns if it contains an exception? The or parameter. Errored does not include excepted, but excepted includes errored because an error code is emitted as an exception_ptr to a system_error containing that error code.

Hugh! Too complex.

...

* Missing overload for get_unexpected. In addition it doesn't return an unexpected_type.

constexpr const E & get_unexpected () const; I didn't realise there were rvalue, lvalue, const rvalue and const lvalue overloads. That must be a recent thing. Logged to https://github.com/ned14/boost.outcome/issues/33.

The return by E rather than expected is by design. Remember we permit implicit construction from E, so this is safe and works well. Again, code written for LEWG Expected should never notice this implementation detail.

Why? if the signatures doesn't match I will have sure a problem somewhere.

(the real cause for this deviation is because the policy classes cannot return types still being fabricated, so they cannot return an expected. If this deviation ever should become a problem, I can relocate get_unexpected to the main class and enable_if it, or else use a trampoline type. But I believe the existing form replicates the same semantics, it should be fine)

This is what I don't like of your library. In order to have a common implementation you sacrifice the concrete interfaces. I prefer to have concrete classes that provide the interface is the most adapted to these classes and then have an adaptation to some type of classes requirement when I need to do generic code. This something that I remarked since the beginning of your library. The basic_monad class is over-engineered IMHO.

Vicente thanks very much for such detailed feedback. It was very valuable.

Niall, please, don't wait until the review is finished to tell us how the issues will be fixed. You could do it on each issue and come back in this ML. Best, Vicente