[boost] [hana] Formal review

Subject: [boost] [hana] Formal review
From: Christophe Henry (christophe.j.henry_at_[hidden])
Date: 2015-06-23 15:46:55

Dear all,

Following is my review of Hana.

- Whether you believe the library should be accepted into Boost
Yes, unconditionally.
Having only one compiler compiling it is not the library's fault and it
should not be held against it.
If possible, I'd like to have some gcc support (and a clear note of what
can be used with gcc).

- Your name
Christophe Henry

- Your knowledge of the problem domain.
Good knowledge of TMP, MPL and Fusion through my authoring of MSM.
No expert of C++14

- What is your evaluation of the library's:
  * Design
Very nice and interesting.
What really slapped me in the face is writing this:
template <typename ...T>
auto smallest = minimum(make_tuple(type<T>...), [](auto t, auto u) {
 return sizeof_(t) < sizeof_(u);
});

Instead of with MPL:
template <typename ...T>
struct smallest
 : deref<
     typename min_element<
         vector<T...>, less<sizeof_<_1>, sizeof_<_2>>
         >::type
    >
{ };

Ouch! I had forgotten how hard MPL can be...

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:
typedef typename ::boost::mpl::fold<
            stt_simulated,mpl::vector0<>,
            ::boost::mpl::push_back< ::boost::mpl::placeholders::_1,
                                     make_row_tag<
::boost::mpl::placeholders::_2 , BaseType > >
        >::type type;

Became:

static auto fold()
        {
            return
                boost::hana::fold.left(stt_simulated{},
boost::mpl::vector0<>{},
                [](auto vec, auto t) {
                   return typename ::boost::mpl::push_back<
                     decltype(vec),
                     typename make_row_tag <typename decltype(t)::type,
BaseType>::type>::type{};
            });
        }
        typedef decltype(fold()) type;

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 ;-)

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.
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.

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.

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

  * 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.
- 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