On Sat, Jul 15, 2017 at 3:19 AM, Bjorn Reese via Boost wrote:

* Documentation: https://rawgit.com/pdimov/mp11/master/doc/html/mp11.html

Is there a rationale explaining how mp11 differentiates itself from the numerous other metaprogramming libraries, some in Boost? Why does Boost need yet another one of these things?

On Sat, Jul 15, 2017 at 3:19 AM, Bjorn Reese via Boost wrote:

The formal review of Peter Dimov's Mp11 library is scheduled for July 15 - July 24, 2017 [1].

* The documentation states: "mp_if_c is an alias for T. mp_if_c is an alias for E. Otherwise, the result is a substitution failure." Should that read "mp_if_c"? If not, why? And shouldn't the document address it?

This is my review of the mp11 library. I. DESIGN --------- The design of the library is well organized and well thought out. The rationale of prepending `mp_` to all identifiers makes sense and results in less confusion. It lets lets you write using namespace boost::mp11; with little chance of stepping on anything in the enclosing namespace. Function names are consistent with their run-time equivalents. The decision to recognize almost any class template as a list is brilliant and speaks to the power of the library to express ideas in flexible and concise ways. II. IMPLEMENTATION ------------------ I looked at each public header file. Its really amazing how C++11 allows the compact expression of metafunctions using parameter packs; this is reflected in mp11's implementation. Many of the functions are simple, intuitive one-liners. There are helpful comments such as this, to aid users who get compilation errors: https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... Everything is `constexpr` where possible, and the author has implemented straightforward optimizations to reduce compilation times for the heavier functions, such as these: https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... A casual inspection of the source files should be enough to give any potential user confidence in the quality of the library (assuming they are somewhat familiar with the domain). The implementation is in sharp contrast to the "template soup" I usually encounter when looking at Boost source code. For example, here's Hana: https://github.com/boostorg/hana/blob/e507494b5f7f51026e8afa317b3ca2c058a4d2... and here's mp11 https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... III. DOCUMENTATION ------------------ The documentation is spartan and not for a beginner like me. There is little explanation on many of the functions (most are single sentences), and a dearth of examples. Most of the reference is a laundry-list of valid expressions with minimal exposition. I found this in a surprising contrast to the linked articles, which were brilliantly written, engaging, informative, and exciting. The author explained that more examples are coming for the reference items. Here's an example of the disparity. The documentation for mp_all has a lot of exposition relative to other reference items, and it includes a good chunk of example code: mp_all mp_all is mp_true if mp_to_bool<U> is mp_true for all types U in T…, mp_false otherwise. Same as mp_and, but does not perform short-circuit evaluation. mp_and is mp_false, but mp_all is an error because void does not have a nested value. The upside is that mp_all is potentially faster and does not mask substitution failures as mp_and does. Code Example 62. mp_all behavior https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... Now compare that with the documentation for mp_replace_at_c: mp_replace_at_c Replaces the element of L at zero-based index I with W and returns the result. https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23... I realize that seasoned metaprogrammers will likely have no trouble at all understanding these terse descriptions and lack of examples but they aren't doing beginners any favors. There are things missing from the documentation that I would expect or hope to find, as some of them may be important to potential users and others are just good marketing which could help to revive the dying Boost: * Comparison to other libraries - How is this different from Hana? MPL? * Limits - Which functions work with almost limitless type lists? - Which functions might require care in the size of type lists? * Speed - Benchmarks. The doc could link to http://metaben.ch/ - Which functions might become slow? The type list limits are compiler-dependent so exact numbers aren't expected, but at least guidance for where someone might encounter problems could be provided to aid in the inevitable troubleshooting. In my opinion, the documentation under-sells the library, I think more could be done to increase the conversion ratio (the fraction of users who decide to use the library after reading the documentation). I can see that metaprogramming gurus who read the Overview will immediately see the advantage of allowing L to be any class template, but I rather doubt that beginners will see that - this should be explained in a bit more direct terms. Here's a rough draft with some sizzle: "mp11 is easier to use than other metaprogramming libraries because it leverages C++11 and allows the use of any class template to express a type list. Programs that use mp11 over other libraries will be easier to read, compile faster, and require fewer lines of code." IV. PRACTICAL USE ----------------- The library appears useful for its intended purpose. I looked through my own code to find metafunctions that could be expressed using mp11 instead. The result of this work can be found here: https://github.com/vinniefalco/Beast/tree/mp11 https://github.com/vinniefalco/Beast/commit/d0385e70fa45c675c617dcd192426e05... I stumbled a bit in the documentation trying to figure out how to form my expressions but once I did the library performed wonderfully. I liked how `mp_repeat_c` was a more general replacement of my `repeat_tuple`. I spent about two hours with it. Its a shame that so few of Beast's metafunctions fell into the domain of problems that mp11 solves but that's an issue with my library and not mp11. I am somewhat familiar with the problem domain, having done some light manipulation of type lists. V. VERDICT ---------- I have seen various metaprogramming libraries over the years, such as MPL and Hana. Neither of them jumped out at me as something easy to use or immediately helpful. Despite all of my aforementioned complaints regarding the documentation, mp11 remains the ONLY metaprogramming library I have ever had success at in applying to my own programs, and the only metaprogramming library I would consider taking as a dependency. For this reason, mp11 should be ACCEPTED. Thanks

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