[boost] Re: Formal Review: FOREACH macro

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2005-05-01 20:20:15

Hi,

Let me step out of review manager role for a moment and give actual review.

As many of you here I fell in love with this utility macro the moment I 
heard about it. Many would agree that lack of expressiveness in lambda 
expression generation is a major drawback in current STL design. 
Boost.Lambda does address part of the issue. But I still believe 
BOOST_FOREACH is more portable, powerfull and easier to use.

Regrdless to what I am going to say later I believe this macro would be a 
valueable addition and should be accepted.

Here is a couple remarks I wanted to make. First a general one: I believe 
FOREACH is so widely usefull it should work for all compilers we do our 
resression tests (with different levels of compartibility). Actually as some 
of you may noticed I implemented version of FOREACH internally in Boost.Test 
namely because I need portable version, while I liked to use FOREACH. Once 
(if) BOOST_FOREACH implementation is made portable for my purposes I would 
be willing to get rid of my version. As it stands now here are results for 
resress.cpp test (compilers I could test on):

borland 5.5.1   - fail
borland 5.6.4   - fail
cw-8.3              - fail
gcc 3.3.3          - pass
Intel 8.1           - pass
gcc 2.95           - fail
vc 6.5               - fail
vc 6.5 (stlport) - fail
vc 7.1               - pass
vc 7.1 (stlport) - pass
sunpro 5.3       - fail

Now for some specific issues:

1. Rvalues support
I agree it's cool and "magical" - the way you added support for rvalues. But 
I believe it's actually misplaced efforts. IMO support for rvalues brings 
more harm than advantage. I could accidently or mindlessly (using some 
global replacement in sources, for example) introduce rvalue in 
BOOST_FOREACH statement and compiler wouldn't warn me. Now I am paying for 
an unnessesary copying. while exist perfectly good alternative that doesn't 
require it:

my_collection_type my_function();
 ...

my_collection_type const& v = my_function()

BOOST_FOREACH(... , v )
{
}

In addition eliminating rvalues support will signicantly simplify 
implementation and as a result speed up compilation.

2. Usage of Boost.Range

I understand why FOREACH is using Boost.Range, but:
 a) In many (most) cases I (and any other developer now) working with stl 
containers and wouldn't need any extra help from boost.range for FOREACH to 
work properly.
 b) Usage of Boost.Range introduces an extra dependency on ... Boost.Range 
and everything it depends on in its' turn (including all portability 
limitations). I for one couldn't afford this dependency,
 c) Usage of Boost.Range adding a level of indiration and slightly 
complicate am implementation

  I am not going to propose to eliminate this dependency completely. But I 
think we need to make it optional. Users shouldn't pay for what they do not 
need (in most/many cases). Also I believe some old compilers could only made 
to work in such mode. So you may introduce another level of compartibility - 
working only with stl compartible container-lvalue.
  As to what should be the default - it's your call.

3. regress.cpp
  I do not see test program testing all the cases (rvalue canst collections 
etc). You may want to create several test files for each level of 
compartibility

4. auto_any

auto_any staff deserves at least special mentionning in docs (if not 
separate header). If should explain the tecnique and how it could be used

5. type2type

I believe we already have something similar in boost.

6. typeof/encode_type
I believe this tecnique desirves separat eheader and special explantion in 
docs.

7. First elem support
FOREACH could easily add support for the first element detection. I think it 
may be wrty addition.

In any case thanks for making it available for us.

Regards,

Gennadiy