From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2005-05-01 20:20:15
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
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
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.
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
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
I believe we already have something similar in boost.
I believe this tecnique desirves separat eheader and special explantion in
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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk