[review][mp11] Formal review of Mp11

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[review][mp11] Formal review of Mp11

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

Mp11 is a C++11 metaprogramming library for compile-time manipulation of
data structures that contain types. It’s based on template aliases and
variadic templates and implements the approach outlined in the article
"Simple C++ metaprogramming" [2] and its sequel [3]. These articles are
useful background information for the review.

   * Mp11 aims to make simple usage simple, and to support complex usage
     without complicating the simple use cases.

   * Mp11 works on any type-list, whether its own type-list mp_list,
     or standard type-lists such as std::tuple and std::variant, or
     user-defined type-lists.

   * Mp11 works with any meta-function, such as C++11 or Boost
     type-traits, or user-defined type-traits.

Mp11 can be found here:

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

   * Source code: https://github.com/pdimov/mp11/tree/master


Please answer the following questions in your review [4]:

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

  2. What is your evaluation of the design?

  3. What is your evaluation of the implementation?

  4. What is your evaluation of the documentation?

  5. What is your evaluation of the potential usefulness of the library?

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

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

  8. Are you knowledgeable about the problem domain?


[1] http://www.boost.org/community/review_schedule.html
[2] http://pdimov.com/cpp2/simple_cxx11_metaprogramming.html
[3] http://pdimov.com/cpp2/simple_cxx11_metaprogramming_2.html
[4] http://www.boost.org/community/reviews.html

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
On Sat, Jul 15, 2017 at 3:19 AM, Bjorn Reese via Boost
<[hidden email]> 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?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 15, 2017 at 3:19 AM, Bjorn Reese via Boost
<[hidden email]> 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<true, T, E…> is an alias for T.
mp_if_c<false, T, E> is an alias for E. Otherwise, the result is a
substitution failure."

Should that read "mp_if_c<false, T, E...>"? If not, why? And shouldn't
the document address it?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Vinnie Falco 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?

Mp11 differentiates itself from Boost.MPL by being based on C++11 constructs
such as template aliases and variadic templates.

Compared to Boost.Hana, Mp11 offers pure type-based metaprogramming. This
more limited scope in practice translates to more compilers supported and
faster compile times.

If we look outside Boost, compared to Metal, the main difference is that
Mp11 is generic with respect to the List and Number concepts; it works with
std::tuple, std::pair, std::variant, the lot, and it supports type traits of
the is_const variety directly.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Vinnie Falco wrote:

> * The documentation states: "mp_if_c<true, T, E…> is an alias for T.
> mp_if_c<false, T, E> is an alias for E. Otherwise, the result is a
> substitution failure."
>
> Should that read "mp_if_c<false, T, E...>"? If not, why?

No.

The idea here is that mp_if_c can be used in two ways. One is mp_if_c<Cond,
T, E>, which gives either T or E depending on Cond. The other is
mp_if_c<Cond, T>, which is the same as std::enable_if_t<Cond, T> - gives T
when Cond, otherwise a substitution failure.

So

mp_if_c<true, T> is T
mp_if_c<true, T, E> is T
mp_if_c<false, T, E> is E

Aliases can't be specialized, so it's not possible to implement just the
above, which is why it takes a parameter pack for E... and does

mp_if_c<true, T, E...> is T
mp_if_c<false, T, E> is E

which for the three cases we're interested in gives the results we need.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
On 07/15/17 15:47, Peter Dimov via Boost wrote:

> Vinnie Falco wrote:
>
>> * The documentation states: "mp_if_c<true, T, E…> is an alias for T.
>> mp_if_c<false, T, E> is an alias for E. Otherwise, the result is a
>> substitution failure."
>>
>> Should that read "mp_if_c<false, T, E...>"? If not, why?
>
> No.
>
> The idea here is that mp_if_c can be used in two ways. One is
> mp_if_c<Cond, T, E>, which gives either T or E depending on Cond. The
> other is mp_if_c<Cond, T>, which is the same as std::enable_if_t<Cond,
> T> - gives T when Cond, otherwise a substitution failure.
>
> So
>
> mp_if_c<true, T> is T
> mp_if_c<true, T, E> is T
> mp_if_c<false, T, E> is E
>
> Aliases can't be specialized, so it's not possible to implement just the
> above, which is why it takes a parameter pack for E... and does
>
> mp_if_c<true, T, E...> is T
> mp_if_c<false, T, E> is E
>
> which for the three cases we're interested in gives the results we need.

Why not separate the two use case into two primitives? We usually want
one or the other and different tools would make sense, at least for
documenting purpose.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
I think for people who don't use packs often the fact that the E... pack
is being used essentially to make E optional is a source of confusion.

The way it's implemented, you can also pass more than one type in the else
to mp_if_c when the condition is true, but not if it's false.  Maybe you
couuld separate the detail::mp_if_c_impl into 2 specializations for the
true case, one with a single else and one with no else type.  Then passing
multiple else types would always fail substituation whether or not the
condition is true or false.

Then I'd change the documentation to list out those two separate true
cases and make no mention of the E..., or if you want you could say
mp_if_c<bool,T,E1,E2,Es...> will always fail substituation explicitly in
the documentation (but that again just seems like adding confusion.)

- Josh

On Sat, 15 Jul 2017, Andrey Semashev via Boost wrote:

> On 07/15/17 15:47, Peter Dimov via Boost wrote:
>> Vinnie Falco wrote:
>>
>>> * The documentation states: "mp_if_c<true, T, E…> is an alias for T.
>>> mp_if_c<false, T, E> is an alias for E. Otherwise, the result is a
>>> substitution failure."
>>>
>>> Should that read "mp_if_c<false, T, E...>"? If not, why?
>>
>> No.
>>
>> The idea here is that mp_if_c can be used in two ways. One is
>> mp_if_c<Cond, T, E>, which gives either T or E depending on Cond. The
>> other is mp_if_c<Cond, T>, which is the same as std::enable_if_t<Cond,
>> T> - gives T when Cond, otherwise a substitution failure.
>>
>> So
>>
>> mp_if_c<true, T> is T
>> mp_if_c<true, T, E> is T
>> mp_if_c<false, T, E> is E
>>
>> Aliases can't be specialized, so it's not possible to implement just the
>> above, which is why it takes a parameter pack for E... and does
>>
>> mp_if_c<true, T, E...> is T
>> mp_if_c<false, T, E> is E
>>
>> which for the three cases we're interested in gives the results we need.
>
> Why not separate the two use case into two primitives? We usually want
> one or the other and different tools would make sense, at least for
> documenting purpose.
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 15, 2017 at 5:57 AM, Andrey Semashev via Boost
<[hidden email]> wrote:
> Why not separate the two use case into two primitives? We usually want one
> or the other and different tools would make sense, at least for documenting
> purpose.

Now that its been explained, I appreciate that `mp_if_c` has
additional functionality. I think its a mistake to split it up when it
is so elegantly expressed in its current form. I just wish there was
more explanation in the reference for a novice such as myself.

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Andrey Semashev wrote:

> > The idea here is that mp_if_c can be used in two ways. One is
> > mp_if_c<Cond, T, E>, which gives either T or E depending on Cond. The
> > other is mp_if_c<Cond, T>, which is the same as std::enable_if_t<Cond,
> > T> - gives T when Cond, otherwise a substitution failure.
...

> Why not separate the two use case into two primitives? We usually want one
> or the other and different tools would make sense, at least for
> documenting purpose.

One could go either way on that, I suppose. It makes sense because mp_if<C,
T, E> gives you T when C, otherwise E, and mp_if<C, T> gives you T when C,
otherwise <nothing>. "If C, T, otherwise, ..."


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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/include/boost/mp11/list.hpp#L28>

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/include/boost/mp11/detail/mp_count.hpp#L46>

<https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23/include/boost/mp11/detail/mp_append.hpp#L81>

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/e507494b5f7f51026e8afa317b3ca2c058a4d290/include/boost/hana/any_of.hpp#L58>

and here's mp11

<https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23/include/boost/mp11/algorithm.hpp#L861>



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<T…>

    mp_all<T…> 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<mp_false, void> is mp_false, but
    mp_all<mp_false, void> 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/doc/mp11/function.adoc#mp_allt>

Now compare that with the documentation for mp_replace_at_c:

    mp_replace_at_c<L, I, W>

    Replaces the element of L at zero-based index I with W and returns
the result.

<https://github.com/pdimov/mp11/blob/8e1d23b11f25c3864593b6fc5a8449306e17ea23/doc/mp11/algorithm.adoc#mp_replace_at_cl-i-w>

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/d0385e70fa45c675c617dcd192426e059bc7941d>

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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Joaquin M López Muñoz wrote:

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

Well... it's also meant to make it possible to move mp11 into std without
changes. :-)

> 1.3 I find that these metafunctions are missing/misplaced:
>    pop_back

pop_back is not provided for the same reason std::vector doesn't provide
pop_front - it's much less efficient than its siblings, and providing it
could mislead people into thinking that it's equally efficient.

>    at/at_c (should be in list.hpp)
>    insert/insert_c/erase/erase_c (should be in list.hpp)
>    replace_at/replace_at_c

The partitioning is roughly based on whether the operation can be trivially
expressed (f.ex. mp_second<L<T1, T2, T...>> -> T2), or requires a more
elaborate implementation (as all primitives taking an arbitrary index
require.)

>    clear (should be in list.hpp)

mp_clear could be in list, yes.

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

Set operations are only provided for the cases where the generic list-based
algorithm would be unsuitable or less efficient.

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

...

This unfortunately doesn't work in general - as Bruno Dutra explained to
me - because MPL operations on, f.ex. mpl::vector, do not necessarily return
an mpl::vector.

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

The purpose of the _q forms is twofold; one, to enable passing a quoted
metafunction, and two, to be metafunctions themselves (that is, they only
take type parameters.) _c_q would meet the first goal but not the second.
This by itself does not preclude its inclusion, but, while the ability to
omit ::value is convenient, it's not indispensable.

>   - It is not "template class" but "class template".

Yes, it's actually "template class", meaning a class template instantiation.
A template class is a class; a class template is a template. Too subtle,
perhaps, but this is correct terminology.

> 3.2.1 A "Requires" clause should be added to many entries.

That's correct. I'll be nailing the requirements down one by one. The choice
in each case is between a hard error and a substitution failure, and it is
not a trivial choice. (Although Bruno would disagree. :-) )

Thank you for the review!


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Joaquin M López Muñoz wrote:

> 1.7 mp_eval[_xx] functions should accept metafunctions in both their true
> and false branches.

I can't make this work.

#include <type_traits>
#include <tuple>

template<bool C, template<class...> class F, class... T, template<class...>
class G, class... U> using eval_if = void;

int main()
{
    using R = eval_if<true, std::tuple, int, float, std::tuple, void>;
}

prog.cc:4:55: error: template parameter pack must be the last template
parameter
template<bool C, template<class...> class F, class... T, template<class...>
class G, class... U> using eval_if = void;
                                                      ^

One might argue that it ought to work because G would obviously terminate
the first pack, but it doesn't. :-)

mp_eval_if_c<cond, mp_eval_if_c<!cond, void, F, T...>, G, U...> is not
pretty but... it works.

There's also the option of typename mp_if_c<cond, mp_defer<F, T...>,
mp_defer<G, U...>>::type.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> > https://github.com/joaquintides/multi_index_mp11/commit/91f2b32cf5a27b3aeaaf6ceb42959c677410f490

> template<typename TagList>  struct no_duplicate_tags  {  typedef
> mp11::mp_fold<  TagList,  mp11::mp_list<>,  mp11::mp_set_push_back  > aux;

This is mp_unique<TagList>, but...

>     BOOST_STATIC_CONSTANT(
> bool,value=(mp11::mp_size<TagList>::value==mp11::mp_size<aux>::value));  };

... this in its entirety is what Metal calls "distinct", the counterpart to
mp_same<T...> that returns true not when all Ts are the same, but when all
Ts are distinct.

A good argument for adding mp_distinct (or the equivalent list-based
formulation mp_is_set), I suppose.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Before I write a review, I have a couple of questions that are the outgrowth of having been using mp11 since some time after the first two articles of Peter’s.  That should suggest I generally like the library, which I do.

Nevertheless, in my code, I have found need for the following additional metafunctions:

- mp_map_keys: creates a list of keys from a map

- mp_is_map: type trait to determine whether a type is a map, i.e., has a unique set of keys

- mp_is_set: type trait to determine whether a type is a set, i.e., has unique elements

Is there a reason that support for such functions should not be in the library?

I have also found need for a metafunction that takes a map and a set of keys and returns a map with only elements with the selected keys.  This is perhaps more specialized, but I can also see a general use case.

I would appreciate your thoughts on these functions.

Thanks.

Cheers,
Brook


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Brook Milligan wrote:
> Before I write a review, I have a couple of questions that are the
> outgrowth of having been using mp11 since some time after the first two
> articles of Peter’s.  That should suggest I generally like the library,
> which I do.
>
> Nevertheless, in my code, I have found need for the following additional
> metafunctions:
>
> - mp_map_keys: creates a list of keys from a map

With metaprogramming libraries, it's always quite difficult to decide what
to include and what to leave out, as there are so many useful things that
are also one-liners. As everyone has probably surmised by now, I prefer
minimalism, that is, omit by default, only include when a clear need arises.

mp_map_keys, for example, is mp_transform<mp_first, M>.

> - mp_is_set: type trait to determine whether a type is a set, i.e., has
> unique elements

This at the moment is std::is_same<S, mp_unique<S>>, although as I
mentioned, perhaps either mp_is_set or mp_distinct should indeed be
provided.

> - mp_is_map: type trait to determine whether a type is a map, i.e., has a
> unique set of keys

This is mp_is_set<mp_map_keys<M>>, although not quite if M has an element
that is not a list.

> I have also found need for a metafunction that takes a map and a set of
> keys and returns a map with only elements with the selected keys.  This is
> perhaps more specialized, but I can also see a general use case.

template<class M, class L> using mp_map_subset =
    mp_copy_if<M, mp_bind<mp_set_contains, L, mp_bind<mp_first,
_1>>::template fn>;

mp_copy_if_q, when I add it, will eliminate the need for the ::template fn
here.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Joaquin M López Muñoz wrote:

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

The primitives in integer_sequence.hpp are template aliases, so
argument-dependent lookup does not apply to them. The tuple functions are,
well, functions, and when f.ex. make_from_tuple<T>(tp) is called with an
std::tuple, ADL finds std::make_from_tuple because std is an associated
namespace. So the code suddenly becomes ambiguous in C++17.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

> On Jul 16, 2017, at 7:00 PM, Peter Dimov via Boost <[hidden email]> wrote:
>
> Brook Milligan wrote:
>> Before I write a review, I have a couple of questions that are the outgrowth of having been using mp11 since some time after the first two articles of Peter’s.  That should suggest I generally like the library, which I do.
>>
>> Nevertheless, in my code, I have found need for the following additional metafunctions:
>>
>> - mp_map_keys: creates a list of keys from a map
>
> With metaprogramming libraries, it's always quite difficult to decide what to include and what to leave out, as there are so many useful things that are also one-liners. As everyone has probably surmised by now, I prefer minimalism, that is, omit by default, only include when a clear need arises.

Fair enough.  The challenge is figuring out when a clear need actually does arise.  I raised this set in part to see if anyone else speaks up to suggest that they are needed.

> mp_map_keys, for example, is mp_transform<mp_first, M>.

Yes, that was my implementation.

>> - mp_is_set: type trait to determine whether a type is a set, i.e., has unique elements
>
> This at the moment is std::is_same<S, mp_unique<S>>, although as I mentioned, perhaps either mp_is_set or mp_distinct should indeed be provided.

Yes, that was my implementation, too.  It does feel like this is worth including as mp_distinct, but your call.

>> - mp_is_map: type trait to determine whether a type is a map, i.e., has a unique set of keys
>
> This is mp_is_set<mp_map_keys<M>>, although not quite if M has an element that is not a list.

Yes, that was also my implementation.  I just punted on the question of whether or not mp_map_keys would always work.  Given that it might not, this could be motivation for a more robust implementation in the library.

>> I have also found need for a metafunction that takes a map and a set of keys and returns a map with only elements with the selected keys.  This is perhaps more specialized, but I can also see a general use case.
>
> template<class M, class L> using mp_map_subset =
>   mp_copy_if<M, mp_bind<mp_set_contains, L, mp_bind<mp_first, _1>>::template fn>;
>
> mp_copy_if_q, when I add it, will eliminate the need for the ::template fn here.

My implementation was a bit more complicated, because I didn’t quite get how to use mp_bind correctly.  Perhaps more than suggesting that this be implemented, this suggests more complete documentation on bind.  At least to me, this is considerably more complex than some of the simpler metafunctions and therefore warrants more documentation, including examples.

This is likely true for many of the algorithms.

Cheers,
Brook


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Hi,

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

Brigand has been inspired by Peter Dimov's original article which explains the similarities.

There is currently redesign work going on in Brigand. Odin is considering bringing Kvasir magic into Brigand for more speed, which explains why we have been very quiet. We are very willing to collaborate, we are just busy improving the library (and our lives also :-) ).

Peter is most welcomed to take anything he likes from Brigand and incorporate it into Mp11, and I'm available by mail or on cppchat to discuss things if need be.

Kind regards.

-Edouard

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
El 17/07/2017 a las 1:06, Peter Dimov via Boost escribió:
> Joaquin M López Muñoz wrote:
>
>> 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.
>
> Well... it's also meant to make it possible to move mp11 into std
> without changes. :-)

We already have subnamespaces in std (e.g. std::chrono), so your
standardization plan could
be realized as s/boost/std.

> 1.3 I find that these metafunctions are missing/misplaced:
>>    pop_back
>
> pop_back is not provided for the same reason std::vector doesn't
> provide pop_front - it's
> much less efficient than its siblings, and providing it could mislead
> people into thinking that
> it's equally efficient.

Understood.

>>    at/at_c (should be in list.hpp)
>>    insert/insert_c/erase/erase_c (should be in list.hpp)
>>    replace_at/replace_at_c
>
> The partitioning is roughly based on whether the operation can be
> trivially expressed
> (f.ex. mp_second<L<T1, T2, T...>> -> T2), or requires a more elaborate
> implementation
> (as all primitives taking an arbitrary index require.)

I see. My assumption was that list.hpp would contain mp equivalents for
std container
member functions, while algorithm.hpp was home to mp translations of
<algorithm> functions. Maybe an intro for each header in the reference
can help
orient readers.

> [...]
>
>> 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).
>
> Set operations are only provided for the cases where the generic
> list-based algorithm
> would be unsuitable or less efficient.

I'm not sure this observation entirely addresses my question.

* mp_set_find is not provided because mp_find does the job --OK, but
consider my
point 1.5.1 then.
* If mp_set_push_(back|front) is provided (no suitable list-based
algorithm), why not
mp_map_push_(back|front)?

>> Porting from / coexisting with Boost.MPL would be greatly aided by
>> some utility
>> function to convert Boost.MPL sequences to Mp11 lists:
>
> ...
>
> This unfortunately doesn't work in general - as Bruno Dutra explained
> to me - because
> MPL operations on, f.ex. mpl::vector, do not necessarily return an
> mpl::vector.

Something more generic can be provided

   struct mp_mpl_sequence_folder
   {
     template<typename T,typename L>
     struct apply{using type=mp_push_front<T,L>;};
   };

   template<typename MplSequence>
   struct mp_mpl_sequence_impl:boost::mpl::reverse_fold<
     MplSequence,
     mp_list<>,
     mp_mpl_sequence_folder
   >{};

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

with faster specializations for boost::mpl::vector and other MPL sequences.

> [...]
>
>>   - It is not "template class" but "class template".
>
> Yes, it's actually "template class", meaning a class template
> instantiation. A template
> class is a class; a class template is a template. Too subtle, perhaps,
> but this is correct
> terminology.

I fail to see "template class" defined in N4296 (one ocurrence only in
[sequences.general],
and seems an editorial typo as it obviously refers to class templates,
and a handful of
ocurrences of "non-template class"). Additionally:

*  "A metafunction is a class template or a template alias [...]":
s/"template alias"/"alias template"?
* "mp_unique<L> returns a list of the same type as L [...]": being "the
same type" needs
definition. This is found in other places throughout the documentation.

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123
Loading...