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: Abel Sinkovics (abel_at_[hidden])
Date: 2015-05-31 09:03:00

Hi Niall,

Thank you for the review.

On 2015-05-31 14:01, Niall Douglas wrote:
>> - What is your evaluation of the implementation?
> * Doesn't follow Boost naming conventions (macro names, namespace,
> directory layout).
Could you please be more specific about the naming convention issues? 
The intention was to make the adoption easy (mostly search and replace: 
mpllibs -> boost, MPLLIBS -> BOOST) in case of acceptance by using the 
Boost naming convection with a different "master" library name (mpllibs 
instead of boost).

> * Doesn't appear to actually be a standalone library, but some
> internal sublibrary.
It is part of a library with same structure Boost used to have (It looks 
like this should have been pointed out in the announcement). This must 
have caused your confusion about the directory structure (parts that 
look similar).

> * Doesn't use Boost.Build.
>
> * Doesn't make use of Appveyor for MSVC per commit testing, if it
> supports MSVC (I couldn't tell).
It does support MSVC (tested with the 2015 beta as well) except for the 
MPLLIBS_STRING macro. I agree that the documentation could (and should) 
be updated with compiler support information.

> 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've been working on it as part of Mpllibs. In case it gets accepted, 
I'll move it to its own repository.

> * 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?
I was doing the same thing Mpl was doing when I made it "Boost-like" and 
I haven't changed it since then. I agree, that Boost.Test does not add 
value to the compile-time assertions. Removing it should be easy.

>> - What is your evaluation of the documentation?
> * Missing Design Rationale.
Good point, I'll add that.

> * 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.
The list of publications can be moved to a sub-page.
Where are the places, where the doc redirects the reader to papers? The 
only place I'm aware of is the measurements page, where bechmarks (even 
more recent ones) can be added (see my comment below).

> * 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.
The most recent measurements and with optimisations enabled/disabled (no 
precompiled headers) can be found here: 
http://abel.sinkovics.hu/download.php?fn=2014_dsl.pdf, starting on slide 
268. The slides compare multiple DSL embedding approaches, the 
"Compile-time parsing" one is about Metaparse.

I agree, that those benchmarks and other measurements (already done ones 
from academic papers and probably some newer ones as well) could be 
added to the library documentation.

> 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.
I'm happy to fix naming convention-related issues (keeping the 
Boost/Mpllibs difference) if you point me to them. Moving the code to 
its own git repository should be straight forward in case of acceptance.

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

Regards,
   Ábel