Re: [boost] [review][mp11] Formal review of Mp11

Subject: Re: [boost] [review][mp11] Formal review of Mp11
From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2017-07-16 22:22:09

This is my review of Mp11. As I see it, an important use niche for this 
library is to
allow for phasing out of Boost.MPL in C++11 compilers, so my review is 
heavily biased by
this perspective.

To get acquainted with the lib, I made the little experiment of porting 
Boost.MultiIndex
from Boost.MPL to Mp11 , results can be seen at:

https://github.com/joaquintides/multi_index_mp11
https://github.com/joaquintides/multi_index_mp11/commit/91f2b32cf5a27b3aeaaf6ceb42959c677410f490

The process was relatively straightforward, 9x% of it was boilerplate 
substitution of the
form s/[typename] mpl::xx<...>::type/mp11::mp_xx<...>. As for the rest, 
no serious problem
was found. Compile times dropped slightly, maybe around 5-10%, but I 
did't measure
rigurously.

1 DESIGN

Mp11 makes the very sensible choice of coding metafunctions/type lists 
as (variadic)
class/alias templates. From this principle everything else follows quite 
naturally. I also
commend dispensing with Boost.MPL iterators and using indexes instead.

1.1 I really don't like the mp_ prefix. I understand this is meant to 
minimize clashes when
using namespace::mp11, but the same purpose can be served by simply
using namespace mp=boost::mp11. Having to add this pesky mp_ thing 
always got in the
way of seamless porting, for no real benefit. To add more confusion, 
some functions
(those in integer_sequence.hpp and tuple.hpp) don't have the prefix.

1.2 Why are quoted metafunctions codified with a fn member rather than 
Boost.MPL-honored
apply? Why call it quoted metafunctions rather than metafunction 
classes, as Boost.MPL
does?

1.3 I find that these metafunctions are missing/misplaced:
   pop_back
   at/at_c (should be in list.hpp)
   insert/insert_c/erase/erase_c (should be in list.hpp)
   replace_at/replace_at_c
   clear (should be in list.hpp)

1.4 Tuple operations are named differently from their C++14/17 
counterparts to
"avoid ambiguities when both are visible or in unqualified calls". Yet, 
this policy is not
followed in integer_sequence.hpp. I'd rather go the latter way.

1.5 Treatment of sets and maps is very dissimilar. Why mp_map_find but no
mp_set_find? Why mp_set_push_(back|front) but no mp_map_push_(back|front)?
Why mp_map_erase but no mp_set_erase? I think both interfaces should 
have the
same functions, except possibly mp_map_(replace|update).

1.5.1 I think that, for consistency, mp_map_find should return an index
(like mp_find) rather than the element or void.

1.6 Boost.MPL sequences simulate variadic behavior by having 
boost::mpl::na-defaulted
type parameters. As a consequence, the following is somehow unexpected:

std::cout<<mp_size<boost::mpl::vector<int,char>>::value<<"\n"; //prints 20

Porting from / coexisting with Boost.MPL would be greatly aided by some 
utility
function to convert Boost.MPL sequences to Mp11 lists:

   template<typename MplSequence>
   struct mp_mpl_sequence_impl;

   template<template<typename...> class M,typename... T>
   struct mp_mpl_sequence_impl<M<T...>>
   {
     using type=mp_remove<mp_list<T...>,boost::mpl::na>;
   };

   template<typename MplSequence>
   using mp_mpl_sequence=typename mp_mpl_sequence_impl<MplSequence>::type;

   ...

   std::cout<<mp_size<
mp_mpl_sequence<boost::mpl::vector<int,char>>>::value<<"\n"; // prints 2

1.7 mp_eval[_xx] functions should accept metafunctions in both their 
true and false
branches. As it stands now, we'd have to write stuff like

   using l1=mp_if_c<
     b,
     mp_list<mp_quote<F>,type1,type2>,
     mp_list<mp_quote<G>,type3,type4>>;
   using l2=mp_apply_q<mp_first<l1>,mp_rest<l1>>;

to emulate (the as of yet non-existent)

   using l2=mp_eval_if_c<b,F,type1,type2,G,type3,type4>;

The current behavior would be served by the new interface with little 
overhead:

   (current) mp_eval_if_c<b,T,F,U...>
   (new) mp_eval_if_c<b,mp_identity_t,T,F,U...>

I have to say I don't know how to apply this new behavior to mp_eval_if_q;
thinking out loud, we'd need something like

   mp_eval_if_q<B,Q1,U1...,mp_else,Q2,U2...>

which doesn't look too pretty. A more exotic alternative could be

   mp_eval_if_q<B,Q1(U1...),Q2(U2...)>

1.7.1 Why there's no eval_if_q_c (or eval_if_c_q?)

2 IMPLEMENTATION

I had just a cursory look at the code, but everything seems clean and 
reasonably
straightforward. I like the fact that C++14/17 features are taken 
advantage of when
available (e.g. variadic fold expressions). There are many lines wider 
than 80
chars, but I think this is OK with current Boost guidelines.

Testing seems pretty exhaustive. I find it surprising that there's so much
boilerplate code repetition in the testing code --I'd have expected Mp11 
itself to
be used to generate extensive test cases automatically.

3 DOCUMENTATION

This is IMHO the weakest part of this submission. I have concerns with 
both the
tutorial and the reference.

3.1 TUTORIAL

With template metaprogramming, one has to ask whether a tutorial for a
metaprogamming lib should be oriented towards explaining *metaprogramming*
or rather the particular design of the library. Mp11 documentation seems to
try both ways, and does not excel in either of them.

3.1.1 Examples are heavy handed and do not focus on why Mp11 is useful 
--too much
discussion goes into the particular difficulties of the use cases 
studied. I consider
myself a reasonably advanced metaprogrammer and found it difficult (and, 
ultimately,
useless) to follow the exposition. For a newbie, the examples are just 
impenetrable; for
a seasoned metaprogrammer wanting to learn Mp11, she's better off 
reading the
definitions section only and then jumping into the reference. I suggest 
taking a look
at Brigand tutorials, which are excellent at explaining the lib to 
entry-level
metaprogrammers in a step-by-step fashion. I think the key is Brigand 
tutorials
don't try to tackle industry-grade metaprogramming problems: they just 
accompany
the reader along toy use cases, and that's really enough.

3.1.2 The definitions section, by contrast, is too terse and at points 
vague to the
verge of incorrection:

   - It is not "template class" but "class template".
   - It is not made clear whether a list is a class/alias template (say, 
mp_list) or
   rather an instantiation of this template (mp_list<int,char>). Same 
goes for
   metafunctions. The confusion extends to the reference, where class 
templates and
   their instantiations are named the same, for instance:

     template<class L> using mp_reverse = /*...*/;
     mp_reverse<L<T1, T2, …​, Tn>> is L<Tn, …​, T2, T1>.

   - The above is not a legal subtlety: as it stands, the definition for 
set doesn't
   make sense, as it implies that a set is a *class template* that somehow
   enforces unicity of template type parameters. Same goes for maps.
   - All in all, seems like the author's intention is to define lists 
(and sets and maps)
   as *template class instantiations*: if this is the case then it has 
to be explained
   how two different lists (say, the result of pushing back a type to a 
list) are connected
   through their common class template.
   - By contrast, seems like the intention is to define a metafunction as a
   class/alias template rather than its instantiations (e.g. mp_size is 
a metafunction
   but mp_size<my_list> is not). If this is the case, then the assertion 
that

     "Another distinguishing feature of this approach is that lists 
(L<T…​>) have the
     same form as metafunctions (F<T…​>) and can therefore be used as such."

   is false.
   - There's a latent concept that goes undocumented, namely lists resulting
   from the instantiation of a non-variadic class/alias template (e.g. 
std::pair). Some
   Mp11 metafunctions are not applicable to them (e.g. mp_push_back), 
but this
   is not clearly documented.

To be clear, I'm not advocating mathematical rigor, but the current 
fuzziness is
confusing at best and can impede understanding at worst.

3.2 REFERENCE

In general, the reference is too terse and overlooks important details.

3.2.1 A "Requires" clause should be added to many entries. For example,
it is not clear what happens with mp_at_c<L, I> when I is out of bounds
(it does not compile). Another example: mp_pop_front<std::pair<int,bool>>
does not compile because std::pair<int,bool> is not a "variadic sequence"
(missing concept here).

3.2.2 A complexity section should be added, where complexity is number of
of template instantiations. For instance, I don't know how costly set 
operations
are.

3.3 PERFORMANCE

As already suggested in another review, links to metaben.ch could be added.

4 WHY ANOTHER METAPROGRAMMING LIBRARY?

The landscape of C++ metaprogramming libs is quite populated. These libs
can be classified into (to follow metaben.ch's terminology) heterogeneous
(Boost.Fusion, Boost.Hana) and pure-type (Boost.MPL, Mp11, Brigand, Metal,
Kvasir). To be fair, we're dealing here with acceptance of a library 
into Boost,
so we should only be really concerned about intra-Boost clashes, which 
leaves
us with Boost.Fusion, Boost.Hana, Boost.MPL and Mp11. In my opinion,
heterogeneous and pure-type libs have different scopes and entry barriers:
for the kind of intra-lib scaffolding I find myself doing, I'd rather go 
with
a pure-type metaprogramming lib before buying the complexities of 
Boost.Hana.
In this respect, a replacement for old and clunky Boost.MPL is most welcome,
and this is the place that Mp11 can occupy.

This said, we can also look out to the wider world and recognize that 
Mp11 and
Brigand have the very same design philosophy and come up with solutions
that are exactly interchangeable. It would be great if we could find a 
way that
efforts behind Mp11 and Brigand could be coordinated to come up with
something bigger that could serve as Boost's pure-type metaprogramming 
library
as well as a non-Boost lib. To be honest, I don't have a clue how this 
could be
done, or whether there's willingness to collaborate among Mp11's and
Brigand's authors.

5 ANSWERS TO REVIEW QUESTIONS

* Should Mp11 be accepted into Boost? Please state all conditions for 
acceptance
explicity.

My vote is for CONDITIONAL ACCEPTANCE dependent upon proper addressing of,
at least,

   mp_list<_1_1, _1_2, _1_4, _1_5, _1_7, _3_1_2, _3_2_1>

Of course I'd like all points of my review to be discussed, but I feel 
only those listed
above are important enough to hold acceptance.

* What is your evaluation of the design?
* What is your evaluation of the implementation?
* What is your evaluation of the documentation?
* What is your evaluation of the potential usefulness of the library?

All addressed above, hopefully.

* Did you try to use the library? With what compiler? Did you have any 
problems?

Yes. VS 2015. No problems.

* How much effort did you put into your evaluation? A glance? A quick 
reading?
In-depth study?

5-6 hours for the porting exercise, 4-5 hours for the  review.

* Are you knowledgeable about the problem domain?

I've been metaprogramming for 14 years.

Thanks to Peter Dimov for his submission of Mp11 and for being such an 
active
contributor to Boost during so many years.

Joaquín M López Muñoz