Re: [boost] [hana] Formal review

Subject: Re: [boost] [hana] Formal review
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2015-06-24 13:33:48

Christophe Henry <christophe.j.henry <at> gmail.com> writes:

>
> Dear all,
>
> Following is my review of Hana.
>
> [...]
>
> However, being value-based instead of type-based turns out to be a strength
> but also a weakness.
> To test the library, I replaced some simple MPL-based implementation parts
> of MSM with Hana. It works, but the code is not as simple and clean as I
> had hoped.
> For example:
> [...]
>
> I had to create object where I actually just wanted types and also a static
> function so I can decltype. This is because this code was written within a
> struct (serving as metafunction). Honestly, my win in readability is
> limited

It seems to me like this is testing the MPL/Hana interoperation rather than Hana
itself. Of course, that interoperation is part of Hana and it ought to work well,
but mixing MPL and Hana styles together should not be expected to give incredible
results. Here's how I would have implemented the above metafunction in pure Hana:

  // This wouldn't be part of the metafunction, because you would ideally
  // receive objects instead of types.
  constexpr auto make_row_tag_v = hana::metafunction<make_row_tag>;
  constexpr auto stt_simulated_v = hana::to<hana::Tuple>(stt_simulated{});

  static auto fold() {
    return hana::fold_left(stt_simulated_v, hana::tuple_t<>, [](auto ts, auto t){
      return hana::append(ts, make_row_tag_v(t, hana::type<BaseType>));
    });
  }
  typedef decltype(fold()) type;

However, the following would also work (and be much more efficient):

  static auto fold() {
    return hana::unpack(stt_simulated_v, [](auto ...t) {
      return hana::make_tuple(make_row_tag_v(t, hana::type<BaseType>)...);
    });
  }

Now, this implementation gives us a hint about how we might want to proceed
for a Hana/MPL implementation:

  static auto fold() {
    return hana::unpack(stt_simulated{}, [](auto ...t) {
      return mpl::vector<
        typename make_row_tag<typename decltype(t)::type, BaseType>::type...
      >{};
    });
  }

Regarding the need for a static function, this would be unnecessary if
(1) the enclosing metafunction was implemented as a function too
(2) lamdbas were allowed in unevaluated contexts

> But my problems started earlier. The heart of MSM is the transition table:
> struct transition_table : mpl::vector<
> Row < Stopped , stop        , Stopped , none                      ,
> none                 >,
> ...
> >{}
>
> All states and events being types, using Hana would lead me to write:
> Row < Stopped{} , stop{}        , Stopped{} , none{}                      ,
> none{}                 >
>
> which would be a regression.

In a potential MSM rewrite with Hana, you could write something like:

  auto transition_table = table(
    row(...)
    ...
    row(...)
  );

I don't know much about MSM, but I think you must have callbacks somewhere in
your transition table, right? One advantage of Hana there would be the ability
to write something like

  auto transition_table = table(
    row(..., [](auto event) {
      ...
    })
  );

> I'd love to get rid of my mpl::vector but Hana won't help me there.
> Maybe MSM was the wrong test object.
>
> The lack of support for mpl::set reduces to a very small part the
> algorithms I could port to Hana. Louis seems to want to change this, which
> I can only welcome.
>
> In the end, I feel that using Hana with MSM would be a take all or nothing
> choice.
> With the current compiler support, it is not something I will do in the
> next few months.

IMHO, the morale is that porting existing code to use Hana is possible, but not
necessarily easy depending on how pervasively you want to use Hana. The primary
goal of the library was _not_ to make it easy to port old code, but rather to
make new libraries much easier to write.

> Still, the library has a nice design and potential and I will probably use
> it either later in other use cases (or with MSM with better support of MPL).
>
>   * Implementation
> I had a short glance and found it very clean.
> I found only one thing which bugged me, the use of non fully qualified
> names in some places. True, many other boost libraries do the same and it
> brings a lot of problems (anyone who tried to include Geometry and
> TypeErasure in one TU will love the duplicate definition of use_default).
> Still, a "return true_;" is begging for conflicts.

I use qualified names when calling functions to avoid ADL-related problems, but
I wasn't aware of potential problems with using `return true_` (or similar).
`true_` is defined in the `boost::hana` namespace, so shouldn't I be able to
refer to it as `true_` unambiguously from within that namespace?
What is the catch?

>   * Documentation
> Great! Way above Boost level.
> As others said, please use fully qualified names where possible. It makes
> it much easier to follow.

Will do.

>   * Tests
> Did not look.
>
>   * Usefulness
> Very useful if you have objects in the first place, maybe less to do TMP.
>
> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
> Clang 3.5.
>
>   * What was the experience? Any problems?
> I got a few compiler errors.
> - A static_assert "hana::fold.left(xs, state, f) requires xs to be
> Foldable" when forgetting to #include <boost/hana/ext/boost/mpl/vector.hpp>
> I had included boost/hana.hpp but it did not include all, which is annoying.

The problem is that if I start including external adapters from <boost/hana.hpp>,
and if I support more than only MPL/Fusion/std, those dependencies are passed
on to the user, which is a problem. For example, if Eric's Meta library is
supported in the future, you shouldn't be required to add this dependency to
your project all of a sudden.

> - a name conflict with std::size_t, though I deserved punishment by adding
> a using namespace boost::hana before including other headers.
>
> - How much effort did you put into your evaluation of the review?
> 8 hours reading the doc and rewriting some MSM algorithms with Hana.
>
> Thank you Louis for writing this excellent library and thank you Glen for
> managing the review!
>
> Christophe

Thanks a lot for your review, Christophe.

Regards,
Louis