views:

310

answers:

6

Ok, someone tell me which would be better. I need to |= the elements of one vector with another. That is, I want to

void orTogether(vector<char>& v1, const vector<char>& v2)
{
    typedef vector<char>::iterator iter;
    for (iter i = v1.begin(), iter j = v2.begin() ; i != v1.end(); ++i, ++j)
        *i |= *j;
}

I can't use for_each due to needing to process 2 collections. I suppose I could do something like

struct BitWiseOr
{
    char operator()(const char& a, const char& b) {return a | b;}
};

void orTogether2(vector<char>& v1, const vector<char>& v2)
{
    transform(v1.begin(), v1.end(), v2.begin(), 
        v1.begin(), BitwiseOr());
}

Is this a more efficient solution even though the top one is in place, but the bottom is an assign? This is right in the middle of a processing loop and I need the fastest code possible.

Edit: Added (obvious?) code for BitwiseOr. Also, I'm getting a lot of comments on non-related things like checking the lengths of v2 and changing the names. This is just an example, the real code is more complicated.

Well, I profiled both. orTogether2 is much faster than orTogether, so I'll be going with the transform method. I was surprised, orTogether2 was about 4 times faster in MSVC9 release mode. I ran it twice, changing the order the second time to make sure it wasn't some sort of cache issue, but same results. Thanks for the help everyone.

+2  A: 

There really isn't much to choose from, until we get to see how MyBitwiseOrObject is implemented. Why don't you run some tests?

The problem with transform here is that you need to have a separate functor and define it at namespace scope (far away from where it is used). Lambda's solve this problem -- you may want to take a look at Boost Lambda Library.

dirkgently
+12  A: 

The bottom one will compile to effectively the same as the first, your OR functor is going to be inlined for sure. So the second idiom is more flexible if you ever need to add more flexibility or debugging frameworks or whatever.

Since there's no benefit to the first, use the transform method. Once you get into that habit you'll stop even considering the explicit loop choice for all your apps since it's unnecessary. The only advantage to the first method is it's easier to explain to beginner C++ programmers who are more comfortable with raw C.

SPWorley
I would suggest making BitWiseOr a template. +1.
strager
+5  A: 

Grab your watch and measure

Robert Gould
+4  A: 

Use transform, methinks.

void orTogether(vector<char>& output, const vector<char>& input)
{
    transform(input.begin(), input.end(), output.begin(), 
        output.begin(), /* binary_or two objects, using boost::lambda would be useful here, as there is only std::logical_or */);
}

Note you should use more descriptive variables names, like output/input rather than 'a' and 'b', helps with your readability, unless that was pseudo-code.

That said, if this turns out to be a slow section of code, you might write it by hand to see what the difference is.

But I doubt it's noticeable or even enough to care about, though. Until you profile you're just guessing, and it's better to write easy to read code.

GMan
A: 

Are you pre-optimizing? Do you know one is faster than the other? I suspect they are rather close to being the same.

That said, I would use the transform. Let the library, which is well tested, do the heavy lifting. Keep your code simple and clean. You may not know exactly what is happing inside a for-each, but you can trust the library designers to have done a good job.

gbrandt
A: 

The algorithms provided by the STL can be assumed to be

  1. correct
  2. implemented reasonably fast
  3. have a execution complexity guarantee (if specified by the standard)

Therefore you can't go wrong using them. In fact STL implementors may be able to specifically use faster implementations based on template specification for some problems.

lothar