[boost] Re: Formal Review: FOREACH macro

From: Eric Niebler (eric_at_[hidden])
Date: 2005-05-02 10:00:14

Gennadiy Rozental wrote:
> 
> 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.

There is an important usage scenario that requires iteration over rvalue 
containers ... when you don't know the type of the container. With the 
ragex_lib (in boost-sandbox), you can filter and transform a range *in 
place* and then immediately iterate over it. Consider:

BOOST_FOREACH(..., v | filter(my_pred()) | transform(my_func()))
{
}

Here, filter and transform are range adaptors, built on top of 
filter_iterator and transform_iterator. The type of the container is 
complicated, but you don't have to mention it anywhere because FOREACH 
correctly handles rvalues.

This is very powerful.

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

Actually, I switched to Boost.Range to make the implementation *less* 
complicated.

> 
>   I am not going to propose to eliminate this dependency completely. But I 
> think we need to make it optional.

What do you mean by and optional dependency on Boost.Range?

  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 need to document how to extend FOREACH. Either it is done by extending 
Boost.Range, or it's done another way. Not sure how an "optional" 
dependeny on Boost.Range fits in to that.

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

Agreed.

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

I consider this an implementation detail. I would rather keep it that 
way until there is a need to break it out, at which point ... maybe a 
fast-track review or something.

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

Also an implementation detail. I would be hesitatnt to break it out 
unless someone demonstrated a need.

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

Putting an extra bool on the stack and maintaining its state is overhead 
that not everyone should have to pay for, IMO. I don't think enough 
people would use it to justify the expense, especially considering that 
it's easy enough to use your own bool to detect the first element:

bool first = true;
BOOST_FOREACH(....)
{
   if(first)
   {
     do_something();
     first = false;
   }
}

> In any case thanks for making it available for us.

My pleasure.

-- 
Eric Niebler
Boost Consulting
www.boost-consulting.com