Re: [boost] [vmd] Library Review

Subject: Re: [boost] [vmd] Library Review
From: Edward Diener (eldiener_at_[hidden])
Date: 2014-09-13 16:51:11

On 9/13/2014 9:44 AM, Paul Mensonides wrote:
> (apologies for the late review)

Your review is most welcome.

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

The use case is to be able to determine if some preprocessor input 
matches a v-identifier and then to logically work off of that decision. 
The idea is to parse some input and, for instance, if the input is X, 
create output A, and if the input is Y, create output B etc. etc., where 
X and Y are v-identifiers.

In any given situation since the v-keys are being
supplied, the end-user knows that if 1 is returned identifier X has been 
matched and if 2 is returned identifier Y has been matched, and of 
course if 0 is returned no identifier match has been found. This enables 
the end-user to dispatch on the result and to know which identifier has 
been found. Admittedly it does not extract the identifier itself as 
output from the macro.

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

Is the ending comma there for a reason ?

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

Ditto about the ending commas.

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

Sure.

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

I appreciate your technique of matching a v-identifier/v-number by 
returning a registered identifier/number and then dispatching off of the 
identifier itself. It appears to make parsing a v-sequence much easier, 
but I need to consider this further.

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

This is a bit unfair. You are asking me to understand Chaos code as 
opposed to Boost PP and VMD. But I think I get the idea from your 
original suggestion above and will study it further.

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

Thanks ! VC++ is horrible to make it work "correctly". But you already 
know that.

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

Please feel free to point them out and I will update the documentation 
accordingly.

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

I don't think that input that ends with a function-like macro name, when 
that macro takes 0 or 1 parameter, causes incorrect results on a C++ 
standard conforming compiler. Can you explain why it would cause an 
incorrect result or an error ?

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

I agree that if I use your methodology of registering v-identifiers and 
v-numbers then the documentation, as well as the code, for parsing 
v-sequences might be much simplified and I may be automatically able to 
parse any v-sequence into its v-types. I will have to take a look at 
this a bit more.

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

Thanks ! I felt the ability to specify/parse a v-sequence would add 
flexibility when designing macros. Also as well the ability to identify 
v-types.

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

Who coined that term anyway <g> ?

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

Thanks very much for your review. I will definitely revisit the 
implementation of v-sequences and the identifying of v-identifiers and 
v-numbers to see if I can make the library better and easier to use 
based on your suggestions.