Re: [review][LEAF] Review of LEAF starts today : May 22 - May 31

From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2020-06-07 09:00:55

El 06/06/2020 a las 22:53, Emil Dotchevski via Boost escribió:
> On Sat, Jun 6, 2020 at 4:21 AM Joaquin M López Muñoz via Boost <
> boost_at_[hidden]> wrote:
>> [...]
>> * I feel like is_result_type should be converted into a traits class
>> (like allocator_traits) with value() and error() static functions
>> so as to make it easier to adapt external error types into the framework.
> Possibly. I prefer to not add complexity that is not strictly needed.

I'd say this doesn't add complexity for the user, but on the contrary 
makes it
easier to detect and adapt result types. I'm thinking about something 
like this:

 Â Â Â  template<typename T,typename=void>
 Â Â Â  struct result_type_traits;

 Â Â Â  template<typename T>
 Â Â Â  struct result_type_traits<
 Â Â Â Â Â  T,
 Â Â Â Â Â  boost::mp11::mp_void<
 Â Â Â Â Â Â  decltype(static_cast<bool>(std::declval<T>())),
 Â Â Â Â Â Â  decltype(std::declval<T>().value()),
 Â Â Â Â Â Â  decltype(std::declval<T>().error())
 Â Â Â Â Â  >
 Â Â Â  >
 Â Â Â  {
 Â Â Â Â Â  static bool has_error(T& x){return static_cast<bool>(x);}
 Â Â Â Â Â  static decltype(std::declval<T>().value()) value(T& x){return 
x.value();}
 Â Â Â Â Â  static decltype(std::declval<T>().error()) error(T& x){return 
x.error();}
 Â Â Â  };

 Â Â Â  template<typename T,std::size_t=sizeof(T)>
     std::true_type  is_complete(T*);
 Â Â Â  std::false_type is_complete(...);

 Â Â Â  template<typename T>
 Â Â Â  using 
is_result_type=decltype(is_complete(std::declval<result_type_traits<T>*>()));

So, if I have a result type that fits the requirements:

 Â Â Â  struct my_result_type
 Â Â Â  {
 Â Â Â Â Â  operator bool(){return true;}
 Â Â Â Â Â  int value(){return 0;}
 Â Â Â Â Â  int error(){return 1;}
 Â Â Â  };

then is_result_type<my_result_type>::value is true automatically and
result_type_traits<my_result_type> works out of the box. When I have 
something that
requires adaptation, specializing result_type_traits suffices:

 Â Â Â  template<>
 Â Â Â  struct result_type_traits<FILE*>
 Â Â Â  {
 Â Â Â Â Â  static bool has_error(FILE* p){return p==nullptr;}
 Â Â Â Â Â  static FILE* value(FILE *p){return p;}
 Â Â Â Â Â  static int error(FILE *p){return errno;}
 Â Â Â  };

 Â Â Â  // is_result_type<FILE*>::value is automatically true

In fact, with this approach is_result_type probably need not be a public 
type.

>> * Using LEAF requires that called functions be wrapped/refactored so as
>> to return a
>> leaf::result<T> value.
> Not a requirement. In fact one of the main design goals of LEAF is to
> support transporting error objects of arbitrary types through intermediate
> uncooperative layers of functions.

Yes, right.

>> Given that users are already expected to use
>> LEAF_AUTO
>> to reduce boilerplate, maybe this macro can be augmented like this:
>>
>>     namespace leaf{
>>     template <typename T>
>>     [[nodiscard]] result<T> wrap_result(result<T>&& x){return std::move(x);}
>>     template <typename T>
>>     [[nodiscard]] result<T> wrap_result(T&& x){return ...;} // figure out what values of T are an error
> Possible, however I'm exploring what I think is a better option to enable
> this functionality. It is possible to make LEAF able use non-result<T>
> types directly, without having to wrap them.

I'd say result_type_traits would enable this, in fact.

>> * Maybe LEAF_AUTO and LEAF_CHECK can be unified into a single, variadic
>> macro. Just
>> a naming observation here.
>>
>> * Maybe there should be a (BOOST_)LEAF_ASSIGN macro to cover this:
>>
>>     LEAF_AUTO(f, file_open(file_name));
>>     ... // closes f
>>     LEAF_ASSIGN(f,file_open(another_file_name));
> Possible, but I'd rather not facilitate reusing of local variables.

Not sure I'm explaining myself... Perfectly valid code (the majority, 
I'd say)
assigns values to local variables after the variable has been constructed.
LEAF_AUTO hides away error handling on construction, but there is no 
equivalent
for assignment. So the LEAF user would have to write workarounds like:

 Â Â  LEAF_AUTO(f, file_open(file_name));
 Â Â  ... // closes f
 Â Â  LEAF_AUTO(f2,file_open(another_file_name));
 Â Â  f=f2;

or worse yet handle error checking explicitly.

>> * Whis is there a need for having separate try_handle_all and
>> try_handle_some? Isn't
>> try_handle_some basically equivalent to try_handle_all with a
>> [](leaf::error_info const & unmatched) handler?
> Two reasons:
>
> 1) try_handle_all ensures at compile time that the user has provided a
> "catch all" handler. Consider that without this requirement, under
> maintenance someone may delete it and now the program has a subtle,
> difficult to detect bug.
>
> 2) because try_handle_all knows all errors are handled, it can unwrap the
> result type and return a value.

I see, thank you. Didn't notice try_handle_all and try_handle_some 
return types
are different, not sure this is a beautiful decision designwise but it's 
ok, but this
is more of a personal opinion I guess.

>
>> * I may be wrong, but I feel like error-handling and exception are
>> completely separate.
>> Is it not possible to have try_handle_all handle both
>> [](leaf::match<...>) and
>> [](leaf::catch_<...>) handers? The framework could statically determine
>> whether there's
>> a catch handler and then insert its try{try_block()}catch{...} thing
>> only in this case, so as to
>> be exception-agnostic when needed.
> That is exactly how LEAF works. Use catch_ in any of your handlers, and
> try_handle_all/some will catch exceptions, otherwise they won't (the third
> alternative, try_catch, always catches exceptions, and does not use a
> result type).

I still don't get it. What is the difference between:

 Â Â Â  int main()
 Â Â Â  {
 Â Â Â Â Â  return boost::leaf::try_handle_some(
 Â Â Â Â Â Â Â  []()->boost::leaf::result<int>{
 Â Â Â Â Â Â Â Â Â  return 0;
 Â Â Â Â Â Â Â  },
 Â Â Â Â Â Â Â  [](boost::leaf::match<int,0>){return 0;},
 Â Â Â Â Â Â Â  [](boost::leaf::catch_<std::exception>){return 1;},
 Â Â Â Â Â Â Â  [](const boost::leaf::error_info&){return 2;}
 Â Â Â Â Â  ).value();
 Â Â Â  }

and

 Â Â Â  int main()
 Â Â Â  {
 Â Â Â Â Â  return boost::leaf::try_catch(
 Â Â Â Â Â Â Â  []()->boost::leaf::result<int>{
 Â Â Â Â Â Â Â Â Â  return 0;
 Â Â Â Â Â Â Â  },
 Â Â Â Â Â Â Â  [](boost::leaf::match<int,0>){return 0;},
 Â Â Â Â Â Â Â  [](boost::leaf::catch_<std::exception>){return 1;},
 Â Â Â Â Â Â Â  [](const boost::leaf::error_info&){return 2;}
 Â Â Â Â Â  ).value();
 Â Â Â  }

(I've verified both compile ok).

Joaquín M López Muñoz