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/91f2b32cf5a27b3aeaaf...
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<>::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 class M,typename... T>
struct mp_mpl_sequence_impl>
{
using type=mp_remove,boost::mpl::na>;
};
template<typename MplSequence>
using mp_mpl_sequence=typename mp_mpl_sequence_impl<MplSequence>::type;
...
std::cout<>>::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_list>;
using l2=mp_apply_q;
to emulate (the as of yet non-existent)
using l2=mp_eval_if_c;
The current behavior would be served by the new interface with little
overhead:
(current) mp_eval_if_c
(new) mp_eval_if_c
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
which doesn't look too pretty. A more exotic alternative could be
mp_eval_if_q
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). 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> is L.
- 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 is not). If this is the case, then the assertion
that
"Another distinguishing feature of this approach is that lists
(L) have the
same form as metafunctions (F) 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 when I is out of bounds
(it does not compile). Another example: mp_pop_front>
does not compile because std::pair 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
