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. _ 1 is a smart ptr, and std::distance did not work on it, I think i needed to use &chunks[0] as the start
2. 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) );

Answer 1

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.

Answer 2

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.

Answer 3

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

Answer 4

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

Answer 5

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.

Answer 6

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.