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

Subject: [boost] [review][mp11] Formal review of Mp11
From: Glen Fernandes (glen.fernandes_at_[hidden])
Date: 2017-07-23 17:44:04

What follows is my review of the Mp11 library:

[Should the library be accepted into Boost?]

Yes. I vote for an unconditional ACCEPT.

[What is your evaluation of the design?]

Excellent. The design where any template can serve as a list, and any template
can serve as a meta-function is both elegant and powerful. The additional
quoted meta-function support with utilities such as mp_bind, mp_bind_front,
and mp_bind_back, addresses the other request I had when reviewing an earlier
version of Mp11. While HOMF were never essential, as the article pointed out,
users appreciate more ways to one-line things without having to write a helper
type. Some additional primitives are desired, as mentioned later in this
review (e.g. min and max functionality) but overall the toolkit is already
vast and useful.

[What is your evaluation of the implementation?

Excellent. The focus on performance is definitely appreciated while we still
have to depend on libraries for many of these meta-programming primitives.
Ideally many of these would be provided in the standard library and have
implementations that only a compiler vendor could provide with built in
support, and offer performance beyond what a library could offer. In the
meanwhile, the performance offered by Mp11 is great, more than sufficient for
my needs, and in my opinion also for the majority of users. In comparison, the
tiny C++11 meta-programming library that I have implemented, uses only the
obvious C++11 implementations for most functionality, and does not offer the
same fast compile times as the equivalents in Mp11. The techniques used by
Mp11 via C++11 constexpr, C++14 constexpr, and even C++17 fold expressions,
besides improving Mp11 performance, were a valuable source to learn from.

Suggestion: Can all the workarounds for MSVC versions 1800 and older for the
empty list cases be avoided by instead using a helper trait? i.e.

  template<template<class...> class L, class... T, template<class...> class P>
  struct mp_find_if_impl<L<T...>, P> {
    using type = typename mp_find_if_impl_2<P, T...>::type;
  };

i.e. Introduce mp_find_if_impl_2 which would then have the obvious
implementation as before, only without requiring the MSVC workaround? This is
only worth considering if you find it to offer some benefit such as:
Additional maintainability of the Mp11 code, with less workarounds, and no
costs, e.g. no increase in compilation times as a consequence.

[What is your evaluation of the documentation?

Very good. The reference documentation is clear, and easily understood. More
example and tutorial content before the reference section would always be
welcome, but not necessary.

Suggestion: The two-part article that Peter wrote (which is referenced in
the documentation) is in my opinion so valuable for users to gain better
understanding and appreciation of the library that it should be included in
the library distribution as part of the documentation. Ideally in the same
Asciidoc form as the current documentation, as a supplemental document (or
Appendix entry).

Suggestion: Does it make sense for the documentation to have a section
describing the conventions the library uses, for potential contributors as
much as users? e.g. If someone was designing an mp_min_element, should it
yield the element type result, or the list index size constant result? Is
there an established convention of when an error is acceptable for an empty
list versus returning an index?

Correction: Under heading mp_find_if<L, P>: "mp_find_f<L, P> is an" should be
"mp_find_if<L, P> is an".

[What is your evaluation of the potential usefulness of the library?

Incredibly useful in its current form, and this is only going to increase as
the library is extended based on user requests for additional functionality.
Almost every meta-programming primitive that I have needed recently either
already exists in Mp11, or I am able to implement using Mp11 with a little
effort. During this review I replaced the use of my own meta-programming
library in a project of mine without any issues. As part of this review I also
tried several exercises where I implemented certain traits and meta-functions
using Mp11. Of all the exercises that I attempted, the only one where the
library lacked the necessary utility was an "mp_min_element" function:

This was an implementation of the typical "type_with_alignment" trait, which
can easily be implemented using a "min_element" functionality. Mp11 does not
have mp_min, mp_max, mp_min_element, mp_max_element presently, and I believe
they should be added because they are useful and users will probably find
themselves implementing it. Sure, mp_sort exists, but I would not use a sort
when a min_element can suffice. In this case, type_with_alignment<N> could use
mp_min_element with a predicate that finds the type in an mp_list of several
scalar types, that matches has an alignment of at least N and has the smallest
size, and then check that the result element type of the mp_min_element is at
least N.

The litmus test for me for a meta-programming library is the question: Would I
use Mp11 in library code? The answer for me is "yes". If it was part of Boost,
I would use it in other Boost libraries (that already require C++11 or
higher).

[Did you try to use the library? With which compiler(s)?
Did you have any problems?]

Yes. I used the library as a replacement for an existing meta-programming
library of mine in real (non-toy, non-exercise) project code that is compiled
with g++ 4.8.5 in c++11 mode for x86 and x86_64 Linux, using Boost 1.64.0, on
64-bit RHEL 6 and measured compilation times. I also used the library in a few
exercises that I compiled with g++ 7.1 and clang 4.0.0, for x86 and x86_64
Linux, using the current Boost master branch, for c++11 and c++14, on 64-bit
Fedora 26, and again measured compilation times. I also compiled and ran the
Mp11 unit tests with the compilers, C++ standard modes, architectures, Boost
distributions, and operating systems mentioned.

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

More than ten hours over this weekend reading the documentation, reviewing the
implementation and tests, using Mp11 in a project replacing my own C++11
meta-programming library, using it in various exercises, and running the Mp11
unit tests with the various compilers, C++ standard modes, targets, and Boost
distributions listed above. Prior to the preparation for the review, I had
spent some time with an earlier version of Mp11 and comparing it to Metal for
implementing a few meta-functions, including the "instantiates_" example from
my library:

  instantiates_<T, templates_<std::set, std::multiset, std::unordered_set> >

Where instantiates_ should be implemented in terms of "is_instantiation_of_":

  is_instantiation_of_<T, std::set>

Which has the obvious behavior indicated by the trait name. The Mp11 approach
provided by Peter for this case, and the relative ease with which I was able
to implement the other meta-functions using Mp11 were a convincing
demonstration of the usefulness of the library.

[Are you knowledgeable about the problem domain?]

Like many of us, I had written a meta-programming library as C++11 language
features have become available. Before that, if Boost.MPL solved a problem,
more often than not it was convenient and useful enough to just use it, even
if writing my own implementation would result in faster compilation times.
Peter's article, however, opened my eyes to a more elegant design, and changed
how I implemented my C++11 meta-programming library, and in general shaped
what I think about meta-programming in modern C++.

Thank you Peter for your work on Mp11, for submitting it for inclusion in
Boost, and for the articles that motivated it and taught the rest of us.

Glen