Re: [boost] Review: preprocessor library

From: Vesa Karvonen (vesa.karvonen_at_[hidden])
Date: 2001-06-29 08:29:55

From: "Douglas Gregor" <gregod_at_[hidden]>
> I believe that the preprocessor library should be accepted into Boost.
>
> As a reasonable testcase for the preprocessor library, I converted
> Boost.Function to use the preprocessor library. I'll relate my experiences
> when performing this conversion first, and then I have a set of specific
> comments.

Thank you very much for your review, testcase and comments.

[snip lots of stuff]

>       typename R BOOST_PREPROCESSOR_IF(BOOST_FUNCTION_NUM_ARGS, \
>                    BOOST_PREPROCESSOR_COMMA,                    \
>                    BOOST_PREPROCESSOR_EMPTY)()                  \
[...]
>                 BOOST_PREPROCESSOR_IF(i,                        \
>                   BOOST_PREPROCESSOR_COMMA,                     \
>                   BOOST_PREPROCESSOR_EMPTY)()                   \
[...]

BTW, you could use BOOST_PREPROCESSOR_COMMA_IF() here.

> ----------Specific Comments--------
>
> Usage & headers:
> - The enumeration of shifted parameters should go from 1 to n, not 1 to
> (n-1). The current behavior is quite surprising and I don't see any
rationale
> behind it.

This can be changed if so desired. I agree that without considering a specific
application of the shifted parameter enumeration macro, this seems like an
inconsistency. Another solution would perhaps be to rename
ENUM_SHIFTED_PARAMS. The following decribes the rationale behind the current
implementation.

The rationale, which I failed to include in the rationale documentation, is
that repetition from 1 to N-1 makes it easier to perform "parameter shifting"
(hence "SHIFTED") of actual rather than formal parameters, which is the
intended application of ENUM_SHIFTED_PARAMS. Parameter shifting is a rather
general technique for implementing metaprogramming primitives that take lists
of types. Parameter shifting can often help to avoid syntactic clutter (while
using) and avoid O(N**N) repetition (while implementing).

Consider the following (longish, but actual) example, which is an
implementation of a "type match" control structure, which is a generalized
version of meta switch case. Among other interesting uses, the type match
control structure can, with suitable predicates and other helpers, be used as
a replacement for the (IF and) SWITCH and EVAL_DEPENDENCY_TABLE primitives
presented in Czarnecki & Eisenecker.

I have added a couple of comments to document the design, but I assume that
readers are familiar with template metaprogramming.

    template
    < class pred // Predicate metafunction object.
                 // This is often same_as<T>, but more
                 // complicated predicates are used
                 // occasionally.
    , class type // Result if the predicate returns true.
    >
    struct type_match_case
    { typedef pred
        pred;
      typedef type
        type;
    };

    template
    < class type
    >
    struct type_match_default // Syntactic sugar for default
    { struct pred
      { template
        < class dummy
        >
        struct code
        { enum
          { value = true
          };
        };
      };

      typedef type
        type;
    };

    namespace detail
    { struct type_match_end;
    }

    template
    < class T
      // Here we enumerate the formal parameters
      // from 0 to N-1
    , BOOST_PREPROCESSOR_ENUM_PARAMS_WITH_A_DEFAULT
      ( BOOST_TMPL_TYPE_MATCH_MAX
      , class C
      , detail::type_match_end
      )
    >
    struct type_match
      : type_inner_if
        < call_code_1
          < typename C0::pred
          , T
          >::value
        , C0
        , type_match
          < T
          // Here we enumerate the shifted actual parameters.
          // We specifically want to enumerate from 1 to N-1.
          , BOOST_PREPROCESSOR_ENUM_SHIFTED_PARAMS
            ( BOOST_TMPL_TYPE_MATCH_MAX
            , C
            )
          >
        >
    {
    };

You can then use it like this (a dummy example):

    type_match
    < int
    , type_match_case< same_as<char>, short >
    , type_match_case< same_as<short>, int >
    , type_match_case< same_as<int>, long >
    , type_match_default< void >
    >::type

As you can see, if the shifted parameter enumeration macro would repeat from 1
to N, it would be necessary to explicitly decrement the
BOOST_TMPL_TYPE_MATCH_MAX before passing it to
BOOST_PREPROCESSOR_ENUM_SHIFTED_PARAMS.

> - Perhaps a "enumerate shifted parameters with defaults" should be added, to
> be consistent with the unshifted version.

This can be done if so desired. Considering:
- preference of 0-based indexing and 1-based naming, and
- the use of shifted enumeration on actual rather than formal parameters,
this issue may need further discussion.

Another solution might be to rename ENUM_SHIFTED_PARAMS to something like
ENUM_SHIFTED_ACTUAL_PARAMS in order to hilite the intent.

However, if there really is use for shifted formal parameters, then IMO,
shifted enumeration with detaults should be supported.

> - A header file "boost/preprocessor.hpp" that pulls in all of the
> preprocessor code would be most appreciated. I found that I was including
> almost every file in the library individually when converting
Boost.Function.

Ok.

> - Each of the headers needs a reference to http://www.boost.org

Ok.

> Documentation:
> - A reference-style section that lists each user-level macro and describes
> its parameters and expansion would be very useful. I found myself looking to
> the code to see what the parameters to BOOST_PREPROCESSOR_REPEAT were.

Ok. This is indeed a big omission in the documentation!

> - The macro names in the tutorial don't have the BOOST_PREPROCESSOR_ prefix,
> but they probably should.

Ok. The intention was to make the examples more readable, by omitting the
prefixes, but it probably makes the examples less readable, and certainly
harder to try them out.

> - A discussion of BOOST_PREPROCESSOR_UNUSED vs. BOOST_PREPROCESSOR_EMPTY
> would be invaluable. Even if a a single sentence in the descriptions of the
> macros they would be used with (i.e., BOOST_PREPROCESSOR_IF) stating which
> one to use would be helpful.

Ok.

> - In the "Notes" for the example "Use a Local Macro to avoid small scale
> repetition", there is a typo in "defining more and different kind of
> operators" ("kind" should be plural).

Ok.

BTW, I think that early fixing of typos in code comments is very important,
because otherwise it may lead to a situation in which otherwise robust headers
are touched just to fix typos, which can lead to unnecessary recompiling.

Thank you very much for the comments!