RE: [boost] PREPROCESSOR library updated

From: Aleksey Gurtovoy (alexy_at_[hidden])
Date: 2001-11-24 12:16:00

Aleksey Gurtovoy wrote:
> Vesa, this is just wonderful! I'll post some detailed 
> comments later, but from what I've managed to grasp 
> from a quick look, the new version of library is 
> amazing (and it was a great library before :).

Ok, here are my comments, in no particular order:

1) I think it's our current convention that the composite library header
that includes everything from "boost/<library_name>" sub-directory should go
into top-level "boost" include directory; given that,
"boost/preprocessor/preprocessor.hpp" should become
"boost/preprocessor.hpp".

2) In the light of our a while-ago discussion of monolithic headers and
compilation times :), I would prefer to see the
"boost/preprocessor/arithmetic.hpp" and "boost/preprocessor/logic.hpp"
headers made composite too (and comparison operations factored out into
separate (and composite :) "boost/comparison.hpp" header), resulting in the
following directory structure:

    boost/preprocessor/arithmetic.hpp
    boost/preprocessor/arithmetic/add.hpp
    boost/preprocessor/arithmetic/sub.hpp
    boost/preprocessor/arithmetic/mul.hpp
    boost/preprocessor/arithmetic/div.hpp

    boost/preprocessor/comparison.hpp
    boost/preprocessor/comparison/equal.hpp
    boost/preprocessor/comparison/not_equal.hpp
    boost/preprocessor/comparison/less.hpp
    boost/preprocessor/comparison/less_equal.hpp
    boost/preprocessor/comparison/greater.hpp
    boost/preprocessor/comparison/greater_equal.hpp

    boost/preprocessor/logical.hpp
    boost/preprocessor/logical/not.hpp
    boost/preprocessor/logical/or.hpp
    boost/preprocessor/logical/and.hpp
    boost/preprocessor/logical/xor.hpp
    boost/preprocessor/logical/nor.hpp
    boost/preprocessor/logical/bool.hpp

Besides conforming to the basic engineering principles we all know about :),
this refactoring would make PREPROCESSOR and 'boost::mpl' directory
structures more alike (in fact, almost identical), which is IMO a good
thing, given the conceptual similarity of the libraries.

3) The first note in the Tutorial's BOOST_PREPROCESSOR_IF says: "THEN and
ELSE don't have to be macros." However, if a least one of the
BOOST_PREPROCESSOR_IF parameters is a function-like macro (see 16.3
[cpp.replace] para 3), and you want it to be expanded _after_ the
BOOST_PREPROCESSOR_IF macro replacement took place, you have to make a
second parameter a function-like macro too. For example, to implement
NUMBERED_EXPRESSION macro, such as NUMBERED_EXPRESSION(n, x) == x if n > 0,
and BOOST_PREPROCESSOR_EMPTY() otherwise, you can't write it like this:

    #define NUMBERED_EXPRESSION(n, x) \
        BOOST_PREPROCESSOR_IF(n, x##n, BOOST_PREPROCESSOR_EMPTY()) \
    /**/

    // not enough actual parameters for macro BOOST_PREPROCESSOR_IF
    void foo(NUMBERED_EXPRESSION(0, int i));

or this:

    #define NUMBERED_EXPRESSION(n, x) \
        BOOST_PREPROCESSOR_IF(n, x##n, BOOST_PREPROCESSOR_EMPTY)() \
    /**/

    // expands to "void foo(int i1())" instead of 
    // intended "void foo(int i1)"
    void foo(NUMBERED_EXPRESSION(1, int i)); 

instead, you have to introduce yet another auxiliary macro to wrap x##n
into:

    #define NUMBERED_EXPRESSION_BODY(x) x##n
    #define NUMBERED_EXPRESSION(n, x) \
        BOOST_PREPROCESSOR_IF( \
              n \
            , NUMBERED_EXPRESSION_BODY \
            , BOOST_PREPROCESSOR_EMPTY \
            )() \
    /**/

and sometimes this could be inconvenient, especially if the expression
doesn't really have to be a macro (there are cases when it does, for
example, if it contains unconditional commas, and wrapping it in parentheses
is not an option). It would be nice for the library to provide an easier way
to handle such (in my experience, quite often) situations. For example:

#define BOOST_PREPROCESSOR_IDENTITY(x) \
    x BOOST_PREPROCESSOR_EMPTY \
/**/

Now, the above example can be rewritten as:

    #define NUMBERED_EXPRESSION(n, x) \
        BOOST_PREPROCESSOR_IF( \
                  n \
                , BOOST_PREPROCESSOR_IDENTITY(x##n) \
                , BOOST_PREPROCESSOR_EMPTY)() \
    /**/

Vesa, what do you think?

4) Bad news - Metrowerks Codewarrior 7.0 has a bug in preprocessor (to be
more concrete, in function-like macro replacement mechanism) that restricts
usage of the library to only very simple cases, at least if you don't write
code that specifically address this issue; for example, the above
NUMBERED_EXPRESSION example doesn't compile on CW 7.0. Below is a simple
test case that reproduces the bug:

    #define IDENTITY_MACRO(x) IDENTITY_MACRO_BODY(x)
    #define IDENTITY_MACRO_BODY(x) x
    #define COMMA_TOKEN() ,

    int a IDENTITY_MACRO(COMMA_TOKEN)() b; // this works
    int c IDENTITY_MACRO(IDENTITY_MACRO(COMMA_TOKEN))() d; // this doesn't

Basically, what's happening here is that function-like COMMA_TOKEN macro
gets expanded _inside_ of the nested IDENTITY_MACRO call - even although
it's NOT followed by a '(' as the next preprocessing token - which is a
clearly an incorrect behavior (see 16.3 [cpp.replace] para 9 for the
detailed description of the function-like macro replacement process). I
haven't submitted bug report yet, but I am going to.

So, this is not a problem of the library, but probably something that needs
to be mentioned in the documentation, may be with some examples of how to
workaround the issue. Just to show one possible way around the problem, here
is a NUMBERED_EXPRESSION macro that does work on MWCW:

    #define NUMBERED_EXPRESSION(n, x) \
        BOOST_PREPROCESSOR_CAT(BOOST_, \
        BOOST_PREPROCESSOR_IF( \
                  n \
                , PREPROCESSOR_IDENTITY(x##n) \
                , PREPROCESSOR_EMPTY \
                ))() \
    /**/

Ok, I guess that's all that I wanted to comment on. Everything else in the
library looks perfect to me, and I am really looking forward to finally see
in CVS! Oh, and as far as I can see, most, if not all, of the formal review
comments have been addressed. Great job, Vesa!

--
Aleksey