Re: [boost] [outcome] High level summary of review feedback accepted so far

Le 29/05/2017 à 00:39, Niall Douglas via Boost a écrit :

...

Niall I'm trying to see what you want to propose by difference to a hypothetical generic interface to see what is the added value. Sure. We are trying to achieve different things though. Your Expected needs to be a superb primitive building block useful for making other stuff. It's why I would be much happier with narrow contracts on your observers, it's easy for someone to derive from expected locally and override narrow observers with wide observers, or add new member functions with wide contracts.

The harder question for you is whether to supply that wide observer alternative yourself. Do you supply a completely different name of type to help public APIs distinguish which contract is being supplied? Or do you provide a second, wide set of observer member functions?

These are hard choices. I will have no problem providing a wide_error() and wide_get_unexpected functions. I believe these are the only missing in expected. We need just to find a good name.

...

...

.has_error() is true if and only if the current state is an error_code_extended instance. Okay. Why don't you name it has_error_code? For me having an exception_ptr stored was also an error. My thinking was that an exception isn't an error. It's *exceptional*. An error is just well, an error. It is not as strong. When I mean an error is not an error_code but any reason the computation failed. Do you have a good name for that? failure()?

...

...

...

Maybe, outcome should have a get_state function that returns an enum so that the user can do a switch. I had previously a .visit(callable) for that. Any sum type should provide a visit function. I've implemented it in the std_make repository. I will add it to Expected proposal once we have a SumType type of classes. My sole reason for leaving it out of the presented library (even though it's actually in the source code, just disabled) was compile time impact. You can easily make do without it. Up to you.

...

...

...

...

(NOTE: expected will track whatever LEWG Expected does, but it differs from what will become result<T> by having a wide contract on .value() and narrow contracts on operator*(), .error(), operator->(). result<T> will have narrow contracts on everything, it is basically a thin wrap of std::variant except with strong never empty warranty) What will be the differences between result<T> and expected?

The wide contracts for the observers? Can not we provide wide and narrow contracts or don't reuse the same name with different meaning? The description above quite literally tells you the differences. Why we cannot provide wide and narrow observers? If this is the single difference I don't see the need for having two types. The whole point behind Outcome's wide observers with default actions is to save typing boilerplate. Typing more code to get more safety seems the wrong way round to me. It should always be the incentive that to get *less* safety the programmer must explicitly type more code.

Now if you agree with that, would the following change to Expected be acceptable:

- value_type& .value() - throws bad_expected_access if not valued

- error_type .error() - returns default constructed error_type if not errored Not for me, but maybe the committee agree with. I will name this error_or. - value_type& .value_raw() - reinterpret_cast We have already operator*. in expected Your library can add it.

- error_type& .error_raw() - reinterpret_cast evidently we don't like the _raw suffix.

If you feel you can reach this, or anything where less safety forces the programmer to type more, I can match that in Outcome. I'm not alone in my case. It will be a consensus of the committee.

...

...

I would assume an expected could only return a std::variant from its .error(). I can't think what else it could do. This function *could* return variant or variant or any sum type that represents the error, that is the Not-A-Value.

Maybe, expected::get_error<Ek> could be more useful. Probably yes.

...

In any case expected should provide a visit() function. Having more than one Error makes this absolutely necessary.

When having more than one error in expected, having access to each one of them using a different interface would make the user code more complex, isn't it? Yes if all kinds of failure are treated as an error.

Outcome distinctly categorises failure-due-to-error and failure-due-to-exception. I think that important given its different use case to Expected which is explicitly for low latency and C++ disabled environments. Do you mean exceptions disabled? How would you have exceptions when the exceptions are disabled?

But Expected as the STL type, I think your proposal is safe. Most of your users will throw exceptions for exceptional situations. I guess that when exceptions are disabled, an implementation will just terminate the program, isn't it?.

...

...

...

...

- New typedefs checked_outcome<T>/checked_result<T>, checked_outcome_e<T>/checked_result_e<T> are added. These mirror the editions just described, but checks and default actions occur on all observer usage so hidden reinterpret_cast<> never occurs. Implicit conversion from non-checked varieties is permitted to checked varieties, but not the other way round. https://github.com/ned14/boost.outcome/issues/47. Examples:

``` // Note result<T> implicitly converts to checked_result<T>, but not // the other way round. So we can use same make_errored_result(). checked_result<Foo> v(make_errored_result(std::errc::invalid_argument)); assert(v.has_error()); // true assert(!v.has_value()); // true // .value() throws std::system_error( // std::make_error_code(std::errc::invalid_argument)); Foo f(std::move(v.value())); ```

``` checked_result<Foo> v(make_valued_result(Foo())); assert(!v.has_error()); // true assert(v.has_value()); // true // .error() returns a default constructed (null) error_code_extended // when result is valued to indicate "no error here" error_code_extended ec(std::move(v.error())); assert(!ec); // true ``` I will need more rationale about the need of this classes and why we need to do an action while observing them. Please, could you elaborate? To save writing boilerplate in the most common use cases.

...

if error returns the stored error or a default error when valued, why we don't use another name for the function? You mean other than .error()? yes.

...

Why do we want to change the signature and the semantic of an existing function (when I say existing, I'm referring so std::experimental::expected, or for ).

Why not have always the same interface and adding an error_or function as suggested in the Expected proposal? This function could be generic as far as we have common interface. Again, less boilerplate.

Expected, because it lets users choose their E type, unavoidably requires more boilerplate. I don't like typing boilerplate, it makes my fingers and wrist hurt more, it clutters my source code, hence Outcome. I prefer to have a uniform interface even if this mean typing more.

I can not ignore this Outcome review, but I have not see too much people saying that the semantic you have done to this functions is the good one. Nevertheless I will add an open point about having narrow and wide contract for the error access. I will add also another open point to have the possibility to return E{} when the contract is wide instead of throwing an exception. IMHO we need just the narrow contract function. The other can be built on top of this by the user or by Outcome. Remember Expected proposal has already two open_points for err error_or(exp, err) and bool has_error(exp, err)

...

What your checked_outcome<T>::error will return if the is a exception_ptr? Ah, finally someone brought this up. Good!

It returns an error_code with bad_outcome_category and bad_outcome_errc::exception_present.

Ugh. Why your design is never coherent. Why you don't name your functions after what they do? What if the exception_ptr contains an arbitrary exception? No way to obtain an error_code :( I believe your design is too much guided by your AFIO use case. Not all the applications use system errors.

This is despite .has_error() being false! Now I think that this is the right way of implementing this, but I'll straight away agree that it is not uncontroversial to me. My argument would be that it is a magic cookie error code to indicate you've done a logic error. Some would say, can't you throw an exception, it is a logic error after all? That would be my second preference if this magic cookie is felt by reviewers to be unwise. I definitely do not think .has_error() should be true if the outcome is exceptioned. And yes, I do get the irony here considering my arguments about not removing the empty state.

I will tell you one thing: in all my nearly two years of using Outcome, I have never yet seen a bad_outcome_errc::exception_present error_code turn up. It has made it hard for me to be sure if this choice is the right call or not.

Whether you have seen them or not it is out of scope. Your program needs to manage them. Vicente