Re: [boost] [Boost-announce] [metaparse] Review period starts May 25th and ends June 7th

Subject: Re: [boost] [Boost-announce] [metaparse] Review period starts May 25th and ends June 7th
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2015-05-31 08:01:36

On 19 May 2015 at 23:39, Christophe Henry wrote:

> - What is your evaluation of the design?

This isn't my field, so I can't say. It looks clever and useful to 
those needing such things (I don't).

> - What is your evaluation of the implementation?

* Doesn't follow Boost naming conventions (macro names, namespace, 
directory layout).

* Doesn't appear to actually be a standalone library, but some 
internal sublibrary.

* Doesn't use Boost.Build.

* Doesn't make use of Appveyor for MSVC per commit testing, if it 
supports MSVC (I couldn't tell).

* I got very confused trying to find his unit testing. The link 
provided was something like 
https://github.com/sabel83/mpllibs/tree/master/mpllibs/metaparse, but 
no unit testing was obvious. I did a github search, and unit testing 
appeared at 
https://github.com/sabel83/mpllibs/tree/master/libs/metaparse/test. 
Now I am confused, because if I go to:

https://github.com/sabel83/mpllibs/tree/master/libs/metaparse

... that does indeed look a bit like a Boost directory layout but 
missing include and src. Which suggests we were given the wrong link, 
or Metaparse is split across two very similar looking paths, or 
something else weird is going on.

So, please please just use the Boost directory layout, and make 
Metaparse a standalone library without all the other stuff.

Let me put this more clearly: If it's not up for peer review, it 
shouldn't be in the git repo.

* I don't get why he's using Boost.Test, and then uses 
BOOST_MPL_ASSERT all over the place? The need for Boost.Test seems 
redundant?

In terms of really standout good things:

* I very much like that he has version namespacing. All Boost 
libraries should do this by default.

> - What is your evaluation of the documentation?

* Not in BoostBook.

* Missing Design Rationale.

* Academic publications shouldn't be on the front page of the docs 
(maybe an Appendix?). I also dislike the frequency the docs say "go 
read this paper for the thing you actually want to know". Just tell 
me what I want to know, don't make me go search a linked paper.

* I have absolutely no idea what compilers, or their versions, this 
works on. I saw a mention of GCC 4.6 working. That's it.

* The benchmarking at 
http://abel.web.elte.hu/mpllibs/metaparse/performance.html doesn't 
show me more than GCC 4.6, nor a graph showing how performance scales 
to complexity. I also want to know performance with optimisation 
enabled, and with precompiled headers enabled. Some mention of 
how much my binaries might bloat would be useful. Extra brownie 
points for some compiler memory consumption figures, as this sort of 
metaprogramming at scale can cause problems for 32 bit systems.

In terms of really standout good things:

* The tutorial is really great. Lots of baby steps with plenty of 
explanation each worked example. I only wish I had the time to do 
this for AFIO.

> - What is your evaluation of the potential usefulness of the library?

Not my domain of expertise.

> - Did you try to use the library?

No.

>  - With what compiler?
>  - Did you have any problems?
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

About an hour reading the docs and the source code.

> - Are you knowledgeable about the problem domain?

No, definitely not.

My personal recommendation is that you withdraw this library from 
review immediately, and go turn it first into an actual standalone 
Boost library complying with Boost naming guidelines in Boost git 
modular format. Then come back to us. Otherwise you're risking the 
rejection due to non-quality problems which would be a shame.

If you proceed with this review with the library in its current 
state, then I recommend rejection as this is not a Boost library in 
its present organisation. Which is a real shame, as the code looks to 
be of high quality, the quick start, user manual and especially 
tutorial are fantastic, and if properly named and laid out and not 
some internal implementation library of a superlibrary, this could be 
a valuable addition to Boost.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ 
http://ie.linkedin.com/in/nialldouglas/