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/