boost lambda or phoenix problem: using std::for_each to operate on each element of a container

Question

I ran into a problem while cleaning up some old code. This is the function:

uint32_t ADT::get_connectivity_data( std::vector< std::vector<uint8_t> > &output )
{
    output.resize(chunks.size());
    for(chunk_vec_t::iterator it = chunks.begin(); it < chunks.end(); ++it)
    {
        uint32_t success = (*it)->get_connectivity_data(output[it-chunks.begin()]);
    }
    return TRUE;
}

What i am interested in doing is cleaning up the for loop to be a lambda expression but quickly got stuck on how exactly I would pass the correct argument to get_connectivity_data. get_connectivity_data takes a std::vector by reference and fills it with some data. output contains a std::vector for each "chunk".

Basically my conclusion for this was that it was substantially easier, cleaner and shorter to leave my code as-is.

EDIT:

So the closest answer to my question as I envisioned it would look was this:

 std::for_each( chunks.begin(), chunks.end(), 
                   bind( &chunk_vec_t::value::type::get_connectivity_data, 
                         _1, 
                         output[ std::distance( _1, chunks.begn() ] 
                       )
                 );

Yet that code does not compile, I made some modifications to the code to get it to compile but I ran into 2 issues:

_ 1 is a smart ptr, and std::distance did not work on it, I think i needed to use &chunks[0] as the start
Since _ 1 is a smart pointer, I had to do: &chunk_vec_t::value_ type::ValueType::get_ connectivity_ data which caused a crash in the VC9 compiler...

The answer regarding zip_ iterators looked good until i read more into it and discovered that for this particular use, the amount of extra code needed was substantial (binding this and that, etc).

EDIT2:

I found an acceptable solution that is both low on extraneous syntax and clear, which I've posted here and below.

std::transform(chunks.begin(), chunks.end(), back_inserter(tmp), boost::bind(&ADTChunk::get_connectivity_data, _1) );

I can't really see what you are driving at here. How could you replace a for loop with a lambda? — 1800 INFORMATION, Oct 22 '08 at 07:07
By using an stl algorithm such as for_each or transform. I didnt "replace a loop" I removed explicit looping from my code which is what I desired. — Raindog, Oct 23 '08 at 19:16
Sure, if you're allowed to rewrite chunk's get_connectivity_data(), then the use of transform is obvious. Let's hope your compiler elides the vector copies. — fizzer, Oct 24 '08 at 05:41
The VC compiler performs the RVO in even debug mode. I am sure for such simple functions, it release builds are able to perform the same optimization. — Raindog, Oct 26 '08 at 01:41

score 3 · Accepted Answer · answered Oct 23 '08 at 19:15

3

After a bit of work I came up with this solution:

std::transform(chunks.begin(), chunks.end(), back_inserter(tmp), boost::bind(&ADTChunk::get_connectivity_data, _1) );

It required that I change get_connectivity_data to return std::vector instead of taking one by reference, and it also required that I change the elements of chunks to be boost::shared_ptr instead of Loki::SmartPtr.

answered Oct 23 '08 at 19:15

Raindog

1,468
3
14
28

BTW, isn't boost::shared_ptr Loki::SmartPtr? I think Loki was inducted into boost. – oz10 Oct 23 '08 at 19:55
@austrig: nope, they are different. Loki is the policy-based design described in Alexandrescu's book. – Adam Mitz Oct 24 '08 at 04:01
Correct. boost::shared_ptr explicitly does not provide a policy based implementation for simplicity reasons. – Raindog Oct 26 '08 at 01:42

score 2 · Answer 2 · answered Oct 22 '08 at 07:32

I think you were correct in thinking that the best thing to do is leave the code as-is. If you're having a hard time understanding it when (a) you're writing it and (b) you understand the exact problem you're trying to solve, imagine how difficult it will be when someone comes along in 3 years time and has to understand both the problem and the solution that you've written.

score 2 · Answer 3 · answered Oct 22 '08 at 15:02

Without seeing the code for the entire class, it is hard to determine what will work. Personally, I think the BOOST_FOREACH is cleaner in this case, but for reference purposes, I might try doing something like this using lambdas (note I can't test compile)

uint32_t ADT::get_connectivity_data( std::vector< std::vector<uint8_t> > &output )
{
    using namespace boost::lambda;

    output.resize( chunks.size() );

    std::for_each( chunks.begin(), chunks.end(), 
                   bind( &chunk_vec_t::value::type::get_connectivity_data, 
                         _1, 
                         output[ std::distance( _1, chunks.begn() ] 
                       )
                 );
    return TRUE;
}

1800 INFORMATION · Answer 4 · 2008-10-22T07:28:24.703

I don't really get what you are driving at with regards to the lambda, but I can make a couple of general suggestions for code cleanup involving STL containers.

Use typedefs of all STL container types:

typedef std::vector<uint8_t> Chunks;
typedef std::vector<Chunks> Output;
uint32_t ADT::get_connectivity_data( Output &output )

Since you are talking about using Boost, use Boost.Foreach:

BOOST_FOREACH(chunk_vec_t::value_type &chunk, chunks)
  uint32_t success =
    chunk->get_connectivity_data(output[std::distance(&chunk, chunks.begin())]);

Stab in the dark on the "lambda" thing:

typedef const boost::function2<uint32_t, chunk_vec_t::value_type, Chunks>
  GetConnectivity;
uint32_t ADT::get_connectivity_data(Output &output, GetConnectivity &getConnectivity)
{
  output.resize(chunks.size());
  BOOST_FOREACH(chunk_vec_t::value_type &chunk, chunks)
    uint32_t success =
      getConnectivity(chunk, output[std::distance(&chunk, chunks.begin())]);
  return TRUE;
}

Then you might call it like:

get_connectivity_data(output,
  boost::bind(&chunk_vec_t::value_type::get_connectivity_data, _1, _2));

score 1 · Answer 5 · answered Oct 22 '08 at 08:32

What you're actually doing is performing an operation on two containers in parallel. This is what boost::zip_iterator is designed for.

However, the only reason you need to process the containers in parallel is that Chunk::get_connectivity_data takes an out parameter. If it were to return by value (using exceptions to report errors), you could just use an insert iterator.

fizzer · Answer 6 · 2008-10-22T19:43:20.710

1

For some reason, STL beginners always insist in using a vector::iterator instead of the more readable (and constant time) operator[]. The expression it-chunks.begin() should have told the original author that he'd already lost the smartass coder game, and needed a humble index after all:

for (size_t i = 0, size = chunks.size(); i < size; ++i)
{
    chunks[i]->get_connectivity_data(output[i]);
}

OP might also consider losing the spurious return code and making this a const member function.

edited Oct 22 '08 at 19:43

answered Oct 22 '08 at 18:57

fizzer

13,551
9
39
61

Before downvoting, put your maintenance coder hat on and compare this loop to the OP's and other posters' versions. – fizzer Oct 22 '08 at 19:19
For the record, chunk_vec_t isn't defined in the sample code... for all we know it is implemented as some other container... though I agree your code is cleanest in the case where it is in fact a vector. – oz10 Oct 22 '08 at 22:36
True, but the fact that 'it-chunks.begin()' compiles is a string hint. – fizzer Oct 23 '08 at 06:04
The reason i down voted your response is not because it was a bad solution, but because I asked explicitly for a lambda/phoenix solution. – Raindog Oct 23 '08 at 18:22
Also note that the only reason i needed the element number was because I had explicitly resized the container, otherwise I could have used an inserter(which i eventually did) – Raindog Oct 23 '08 at 19:23
You needed the index because the original ADTChunk::get_connectivity_data took the copy destination as a reference parameter rather than returning a value. – fizzer Oct 24 '08 at 05:03

boost lambda or phoenix problem: using std::for_each to operate on each element of a container

6 Answers6