[boost] [vmd] Library Review

Subject: [boost] [vmd] Library Review
From: Paul Mensonides (pmenso57_at_[hidden])
Date: 2014-09-13 09:44:31

(apologies for the late review)

-- Design --

Many of the utility components of the library have decent design.  There 
are, however, a couple of aspects of the design that I dislike, and they 
touch on a large chunk of the library.  These concerns all revolve 
around the registration, recognition, parsing, and limitations of 
various "v-types" and "v-sequences".

I do not yet see a strong use case for the "v-key"/"v-identifier" 
recognition scenario, as opposed to just registering a "v-identifier". 
For example,

     #define BOOST_VMD_MAP_<v-key><v-identifier>
     // ...

     BOOST_VMD_IS_IDENTIFIER(<v-seq>, <tuple-of-v-keys>)

Perhaps Edward can enlighten me about the use-cases for which this is 
ideal.  I do see some use-cases, but in most cases it requires dispatch 
based on what the particular "v-identifier" it is anyway.

One could do better for this low-level part with registrations such as 
(without a BOOST_VMD_ prefix for brevity)

     #define IDENTIFIER_CIRCLE (CIRCLE),
     #define IDENTIFIER_SQUARE (SQUARE),
     #define IDENTIFIER_TRIANGLE (TRIANGLE),
     #define IDENTIFIER_RECTANGLE (RECTANGLE),

These definitions don't allow one to directly test for specific 
"v-identifiers", but they do allow one to extract registered 
"v-identifiers" from a "v-sequence".

If all such registrations are required to be the same, there is no need 
for separate per-library prefixes.  Macros can be defined multiple times 
if they are defined the same way.

Similarly, the "v-numbers" could be pre-registered by the library as

     #define NUMBER_0 (0),
     #define NUMBER_1 (1),
     #define NUMBER_2 (2),
     // ...

Tests for specific "v-identifiers" can be done via extraction from the 
above and using macro definitions such as

     #define COMPARE_CIRCLE_CIRCLE ()
     #define COMPARE_SQUARE_SQUARE ()
     // ...

In many cases this sort of comparison is not needed, as one would 
directly dispatch off of the "v-identifier" itself.

In any case, one could "parse" a "v-sequence" into "v-types" without 
many of the limitations of the current design.  The following example 
uses Chaos, but I do not think there is anything in the design that 
fundamentally cannot be made to work on (e.g.) VC++.

     #include <chaos/preprocessor.h>

     #define NUMBER_1 (1),
     #define NUMBER_2 (2),
     #define NUMBER_3 (3),
     #define NUMBER_4 (4),
     #define NUMBER_5 (5),
     // ...

     // parser implementation...
     #define PARSE(vseq) PARSE_S(CHAOS_PP_STATE(), vseq)
     #define PARSE_S(s, vseq) \
         CHAOS_PP_SPLIT(0, CHAOS_PP_EXPR_S(s)(CHAOS_PP_WHILE_S( \
             s, PARSE_P, PARSE_O,, vseq \
         ))) \
         /**/
     #define PARSE_P(s, seq, vseq) \
         CHAOS_PP_COMPL(CHAOS_PP_IS_EMPTY_NON_FUNCTION(vseq)) \
         /**/
     #define PARSE_O(s, seq, vseq) \
         seq CHAOS_PP_IIF(CHAOS_PP_IS_VARIADIC(vseq))( \
             PARSE_O_A, PARSE_O_B \
         )(vseq) \
         /**/
     #define PARSE_O_A(vseq) PARSE_O_A_A vseq
     #define PARSE_O_A_A(...) (3, (__VA_ARGS__)),
     #define PARSE_O_B(vseq) \
         CHAOS_PP_IIF(CHAOS_PP_IS_VARIADIC(NUMBER_ ## vseq))( \
             PARSE_O_B_A, \
             CHAOS_PP_IIF(CHAOS_PP_IS_VARIADIC(IDENTIFIER_ ## vseq))( \
                 PARSE_O_B_B, PARSE_O_B_C \
             ) \
         )(vseq) \
         /**/
     #define PARSE_O_B_A(vseq) \
         (1, CHAOS_PP_REM_CTOR( \
             CHAOS_PP_SPLIT(0, NUMBER_ ## vseq) \
         )), \
         CHAOS_PP_SPLIT(1, NUMBER_ ## vseq) \
         /**/
     #define PARSE_O_B_B(vseq) \
         (2, CHAOS_PP_REM_CTOR( \
             CHAOS_PP_SPLIT(0, IDENTIFIER_ ## vseq) \
         )), \
         CHAOS_PP_SPLIT(1, IDENTIFIER_ ## vseq) \
         /**/
     #define PARSE_O_B_C(vseq) (0, vseq),

     // registrations...
     #define IDENTIFIER_CIRCLE (CIRCLE),
     #define IDENTIFIER_SQUARE (SQUARE),
     #define IDENTIFIER_TRIANGLE (TRIANGLE),
     #define IDENTIFIER_RECTANGLE (RECTANGLE),

     PARSE(
         CIRCLE
         RECTANGLE
         (a, b, c)
         TRIANGLE
         3
         (xyz)
         UNREGISTERED
         SQUARE
     )

(Apologies for any mistakes in the above.  I wrote it up quickly.)

This particular example results in key-value pairs of the form:
(0, <unrecognized>)
(1, <v-number>)
(2, <registered-identifier>)
(3, <tuple>)

(2, CIRCLE) (2, RECTANGLE) (3, (a, b, c)) (2, TRIANGLE) (1, 3) (3, 
(xyz)) (0, UNREGISTERED SQUARE)

There are a number of things that could be done instead of producing a 
binary sequence like this (which Boost.Preprocessor cannot handle).  One 
could fold the results via a user-supplied macro, for example.

Aside from the massive amount of clutter introduced by workarounds, many 
of the pieces of the above are already available either in 
Boost.Preprocessor or in other parts of the VMD library.  For example, 
BOOST_VMD_IS_EMPTY is essentially CHAOS_PP_IS_EMPTY_NON_FUNCTION.

-- Implementation --

I have not looked closely at the implementation, but I do know what a 
herculean effort it is to get this type of stuff working with VC++ in 
particular.

-- Documentation --

There are few inaccuracies in the documentation related to motivations 
for various Boost.Preprocessor things, but those could be fixed.

The documentation for BOOST_VMD_IS_EMPTY is decent except that 
essentially any input that ends with a function-like macro name will 
cause weird results or errors.  So much so that the documentation should 
just disallow input that ends with a function-like macro name across the 
board.

Other than that, the documentation needs some clean-up and organization 
(particularly of definitions), but does a reasonable job of documenting 
the library.  I do feel that a lot of documentation complexity comes 
from the way that v-keys/v-identifiers (etc) are handled.

-- Usefulness --

There is a definite use-case for many of the utilities in the library, 
and the fundamental concept v-sequence parsing.

-- Testing --

I tested the library with GCC and everything functioned as advertised.

-- Effort --

I did a fairly significant study of the design and interface of the library.

-- Domain Familiarity --

I had my skull opened up and had "preprocessor metaprogramming" tattooed 
directly onto my brain.

--
There are many good parts of the library--including the herculean effort 
to make it work on sub-par preprocessors such as VC++.  However, at this 
point I would be hesitant to recommend acceptance based on the design of 
the v-type recognition and v-sequence parsing part.
I am open to argument about why the current design is superior based on 
use-cases or technical limitations introduced by (e.g.) VC++ or, if it 
is not superior, a further review at a later date after a redesign 
because the underlying concept is sound.
--
Regards,
Paul Mensonides