google / fruit

Fruit, a dependency injection framework for C++
https://github.com/google/fruit/wiki
Apache License 2.0
1.81k stars 200 forks source link

[Suggestion] ExtractFirstError meta function SHOULD be non-error safe #155

Open SelcukAydi opened 1 year ago

SelcukAydi commented 1 year ago

I believe that a meta function should be safe to instantiate its internal type. For example, ExtractFirstError has a type member type alias only in the case of an error propagated. This forces the user to be sure that this meta function should be used in the case of an error to get its type member. However, when there is no error at all then this meta function's apply struct's recursive instantiation will be ill-formed. So a suggestion may be as the following:

Current code

struct ExtractFirstError {
  template <typename... Types>
  struct apply;

  template <typename Type, typename... Types>
  struct apply<Type, Types...> : public apply<Types...> {};

  template <typename ErrorTag, typename... ErrorParams, typename... Types>
  struct apply<Error<ErrorTag, ErrorParams...>, Types...> {
    using type = Error<ErrorTag, ErrorParams...>;
  };
};

Suggestion code

struct ExtractFirstError
{
    template <typename... Types>
    struct apply;

    template <typename Type>
    struct apply<Type>
    {
        using type = None;
    };

    template <typename Type1, typename Type2, typename... Types>
    struct apply<Type1, Type2, Types...> : public apply<Type2, Types...>
    {
    };

    template <typename ErrorTag, typename... ErrorParams>
    struct apply<Error<ErrorTag, ErrorParams...>>
    {
        using type = Error<ErrorTag, ErrorParams...>;
    };
};
poletti-marco commented 1 year ago

Hi, the precondition for calling this ATM is that there is an error but i guess alternatively it could also be changed as you mention, the downside of that is that we wouldn't notice if this is accidentally called in a non-error case, but it's not a huge deal.

That said I'm surprised you were looking into this, this is internal Fruit code. From a Fruit user point of view it doesn't matter right? Or, is there a case where this would help / where a Fruit user would notice the difference? That could be a stronger reason to prefer one or the other formulation.

SelcukAydi commented 1 year ago

Calling a meta function may have a precondition in terms of internal code perspective. I totally agree. I just wanted to give a suggestion that follows the best practices. It is just very subjective.

From the perspective of the end-user, this does not matter obviously however, this meta folder might have been more reusable and agnostic. But again you say this is an internal code and I got no counter-argument :) Your code is already readable and easy to understand.