- What is your evaluation of the design? Confusing. Functions with the same name do different things and what
Hi, I was not sure I would take the time to do a review, but given the quantity of exchanges I believe that I must do it. Le 29/05/2017 à 21:51, charleyb123 . via Boost a écrit : they do is to wired for the specific case. It is not clear if empty is a success or a failure of the computation. I believe it is up to the user to decide what it is. I'm not against having an empty state, but I would like to know at looking at the type it is a success case or a failure without reading the application interpretation of it. I want an observer that tell me if the computation succeeds or fails. I believe that has_value was the needed check, but all this depends on what empty means. I expected that error() returned the reason for failure, but I is not the case, and it depends on the type. I believe that even if people don't want operator* to have a narrow contract we must be coherent with optional, and every smart pointer (Possibly Value type). I have no problem with the wide contracts, but we need a narrow one doing the same thing so that we don't pay for what we don't use. I don't believe that the narrow contracts need to be named with a unsafe_ prefix or a _raw postfix. Remember, we don't want to use unsigned int just because is is unsigned, if we don't want overflow to be defined with modulo arithmetic. A bad defined wide operation is not safer than a well defined narrow operation. Functions with different purposes need different names. When you access the error of a result<T> you don't expect to have a new error code in case there was no error. This is error_or. Maybe have some free function when this can be implemented using the minimal interface. Maybe associated to a specific concept. At the end, being "explicit is better than implicit". I've not taken a look at all to the error_code_extended, but I believe this is out of the scope of the library ans is something that could be part of the Boost.System library. The fact Niall don't wanted to depend on Boost (he has his reasons) has as consequence that he reinvent the wheel. The behavior when there is no exception is now the responsibility of Boost.Exception and any Boost library that want to run with exception disabled must use this library. Concerning boost-lite, I have no problem with whatever the library can needs for implementation purpose or test to be in a detail folder. We don't need sub-modules here. When located in this folders this will be part of the implementation evaluation, and not the design. I don't know if the design of a class that must work when exceptions are disabled, don't need really a different interface than a class that is used with exceptions enabled. I've not considered this while designing expected for the standard, as there there is not this possibility. I recognize the utility of these classes on such environments. The definition of the operations must include this case (exceptions disabled) and a rationale must be included to understand why this is the best interface in both cases. E.g. when exceptions are disabled, what is the meaning of value()? Do we really want such operation in those environments? As there are more than two possibilities, the visit() and an index() function should be there so that we could avoid the if-then-else code. The monad word should disappear from the doc and implementation. While these classes are possible monads (or nested monads), the library doesn't propose a monadic interface. The TRY macro is not a monadic interface but a PossiblyValued interface. What I mean is that the macros could work for any PossiblyValued type. It could works for the proposed Outcomes classes as for std::optional, std::experimental::expected. But Boost.Outcome has not this pretensions and so the macro could be used only for the classes defined by Outcome, as we don't have requirements on the types. I wonder how the really functional monadic functions (functor::transform, monad::chain, monad_error::catch_error) would be defined for all these types, in particular for the types that have an empty state and for those that have an error or an exception_ptr?
- What is your evaluation of the implementation?
I've take a diagonal look at, and I believe that this is much more complex than needed. I'll take a look next time if the design change. I'm not saying that implementing this simple classes is simple. It is not. I believe that Niall is right to look for a common class for all this cases. However the internal class is not what I would like to have. I' glad to see that Niall will remove all the preprocessor stuff. I hope that he will adapt his library to Boost so that we need to read less implementation details that are already in other libraries, ad Boost.Config/Predef, Boost.BackTrace and Boost.Exception. If there are missing macros in Boost.Config Niall should request/provide new macros for Config/Predef. At the end I believe we need a never-empty variant class that would help to implement all these classes (something like what Anthony WIlliams proposed for the standard). I understand that Niall prefers to take in account only 2 or 3 specific types, but I'm wondering if this restrictions are really helping to define correctly the library. At least this makes the check by the user more difficult.
- What is your evaluation of the documentation?
I have read the documentation in diagonal. I don't think the documentation describes clearly what is the motivation of this library, what are the rationale for the design decisions and the reference document is more than incomplete. For a rationale for the design decisions I mean for each operation, in which use cases this operation is interesting, what we cannot do without it, ... I've learn a lot about the library from the exchanges of this review. What I have learn, was not evident in the documentation. There are also references to the implementation details in the reference documentation that make his lecture even more difficult. There are serious problem with the Doxygen generated documentation? I'm not again Doxygen, but we need to check it carefully.
- What is your evaluation of the potential usefulness of the library?
I believe we need something like expected
- Did you try to use the library? With what compiler? Did you have any problems?
Not at all. I would do it if I've found the good.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've not counted the time. A lot I guess.
- Are you knowledgeable about the problem domain?
Less than I could think before the review. People have multiple needs and responding in a coherent way to all these needs is hard. We should not forget to KISS, at least at the user level. Some believe that UB is against them. It is not.
And most importantly:
- Do you think the library should be accepted as a Boost library?
No, not now. I'm not for the proposed interface. I find the documentation not addressing the problem and it is incomplete. Maybe later, if Niall decides to go towards something simpler and more coherent, ... The same name for the same functionality, a different name when the functionality changes. I'm almost sure I don't want the 1st nor the 2nd HLS he his proposing. Niall say that the global design has not hanged a lot between the library we are reviewing and the one he is proposing to us if accepted. He is right. He has accepted to suggestions but the global library stands the same. I believe however that his 2nd HLS could have more changes to be accepted, but I don't think this could be a conditional acceptance. Niall has a lot of things to do and the whole library should be reviewed. Best, Vicente