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

Now to the reply to your post point by point ...

...

...

# 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?

You can't directly. But some might wrap it in a unique_ptr. Up to them, it is they who supply type T and they may not even control its implementation. If a user is forced to use such a T they cannot modify, I don't think you should punish them for that. In general I don't think you can rightly impose that T needs to have a move or copy constructor. You don't *have* to impose it, so don't impose it. Don't mess up the end user's life and make their programming harder for no good reason.

...

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?

optional is in the C++ 17 standard. Code will be written against its current design of .value_or(). It can no longer be changed :(

...

...

* 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?

Exactly the same: value_type &value_or(value_type &) value_type is defined to an unusable but helpfully named type for compiler warnings and error when raw_value_type=void. This is very tersely explained in the Expected synopsis page. I will be improving the reference API docs to describe this better as part of issue #26.

...

...

|* |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?

Yes.

...

...

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?

Heh. You are completely right, noexcept is missing. That's a bug: logged to https://github.com/ned14/boost.outcome/issues/34.

...

...

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

The information in the tutorial will be copied into each reference API's docs using a style similar to standardese. I will cover constructors, assignments, destructors etc as well.

...

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.

I found only this in libstdc++'s unit tests for its optional: std::optional o { std::in_place, { 18, 4 }, std::allocator<int> {} }; It'll do.

...

...

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)

?

As I've mentioned to you by private email, a lot of end users want implicit conversion from T and E to save on boilerplate. Outcome doesn't allow T and E to be convertible into one another, so permitting implicit conversion from both T and E is safe. Consider it a populist extension over LEWG Expected. I know you don't like it, and I to be honest think it best used sparingly, but it'll be very popular with many.

...

...

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

When configured with void, value_type, error_type and exception_type are all set to an unusable but usefully named types suitable for compiler warnings and errors. This makes writing metaprogramming much easier as you don't need to deal specially with void and its weird semantics, plus any attempt to use the type causes a very descriptive compiler error. raw_value_type, raw_error_type and raw_exception_type are the true, original type configured. You are correct that this is a deviation from LEWG Expected which would cause code written for LEWG Expected to fail to compile with Outcome's Expected. I think that safe.

...

...

* 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?

It's the same sizeof. If all your types have nothrow move, or your types don't have move nor copy constructors, we directly destruct and construct during assignment without saving the old state as the fast path. Else we move the current state onto a stack based copy before copy or move constructing in the new state. If that throws, we move back the previous state. If that restoring move throws as well, we are left valueless by exception/empty. I didn't implement Anthony William's double buffer solution for perfect never empty warranties even when the types have no move nor copy constructors. It is quite complex if you also wish to get alignment right, constexpr right, and keep the optimiser maximally folding code. I will document the semantics described above in the tutorial and reference API docs. They are a good balance of never empty warranties with space consumption and runtime overhead. I believe these guarantees are similar to those in your Expected proposal? As in, you don't implement the double buffer solution either.

...

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.

Such problems are on me as the maintainer.

...

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

The common implementation is the heart and soul of Outcome, and is precisely its "value add" over most other Either monad or Expected implementations. A common implementation allows seamless interoperation between implementations. It lets you write code using expected with outcome<T>, result<T> and option<T> all using the exact same framework. End users can extend the family with any arbitrary custom error transport of their choosing for some bespoke use case.

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.

In most of my libraries so do I. But in this situation, I went a different direction. I think it's the right one for this use case.

This something that I remarked since the beginning of your library. The basic_monad class is over-engineered IMHO.

Please see the other email for a reply to this sentiment.

...

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.

I try my best to do so. But a lot of time, how the issues will be fixed is very obvious: I copy and paste documentation from one location to another for example, or I repair an incorrect constructor so it is correct and so on. It's not rocket science to deduce how most issues will be fixed. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/