3
\$\begingroup\$

I wrote a small scientific simulation where I wanted to manipulate vectors with the same kind of functionality that Python's NumPy vectors use. Namely, I wanted to be able to add vectors together, multiply by constants, etc. I have need for both vectors of type int and floating point, so I wanted to template the overloading. As an example (and for something to review), I overloaded scalar multiplication as:

template <class T, class Q>
vector <T> operator*(const Q c, const vector<T> &A) {
  vector <T> R(A.size());
  for(int i=0;i<A.size();++i) 
    R[i] = c*A[i];
  return R;
}

The STL algorithm way I hinted at looks like this:

template <class T, class Q>
vector <T> operator*(const Q c, const vector<T> &A) {

  vector <T> R(A.size());
  std::transform(A.begin(), A.end(), R.begin(),
                 std::bind1st(std::multiplies<T>(),c));
  return R;
}

I would appreciate all criticism relevant to code, style, flow. My project is in C++11, so in theory I could use the newer range-based for loops for readability (I do in other parts of my code). Additionally, I think I could rewrite this using std::transform, but I'm not sure if it would help improve the efficiency or the readability of my code.

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

I would use the second version that you posted. Here are some problems with the first version:

1. Prefer passing by value rather than making an explicit copy.

This is a problem in both versions of your code. You pass A via constant reference. Normally this is a good thing, but then you then make a copy of A explicitly. In this case, it is possible to miss out on a compiler optimization called copy elision (second reference here). Passing by value would not cause any variable aliasing problems.

Update: Imagine somebody (maybe you) using your function.

std::vector <int> v1 = // ... insert elements
auto v2 = 3 * v1 ;

For simplicity, let's say variable v2 is located at address 0x10. When you do 3 * v1, you are calling your function.

R would then be created at let's say address 0x20. When your function returns, the contents of R would have to be copied from 0x20 to v2 at 0x10 and then the destructor for R would have to be called. Without copy elision, this would require copying every element from R into v2.

Now in C++11, there is something called move semantics. This means, that without copy elision, R would not have to copy every element into v2, it could swap pointers. This is much faster, but it is still slower than copy elision.

With copy elision, instead of creating R at 0x20, it uses address 0x10 and builds v2 in place. With copy elision, nothing has to be copied and no pointers have to be swapped because v2 is built in place.

Here's how I would re-write the function:

template <class T, class Q>
std::vector <T> operator* (const Q c, std::vector <T> A)
{
    std::transform (A.begin (), A.end (), A.begin (),
                 std::bind1st (std::multiplies <T> () , c)) ;
    return A ;
}

2. If you have to use an index-loop for some reason, rather than a range-loop or iterators, make sure you are comparing compatible types.

You could do something like this:

typedef std::vector <T>::size_type size_type ;
for (size_type i = 0; i < A.size (); ++i)

C++11 has expanded the functionality of the using keyword. So instead of a typedef, you could also do:

using size_type = decltype (A.size ()) ;

Update: The reason you want to use compatible types is because overflowing a signed number is undefined behavior in C++. This means anything can happen. Consider this example:

const unsigned size = std::numeric_limits <int>::max () + 5 ;
std::vector <char> v1 (size) ;

for (int n = std::numeric_limits <int>::max (); n < v1.size (); ++n) {
    std::cout << n << ',' << v1.size () << '\n' ;
}

Different compilers on different platforms will do different things with the code above because there is undefined behavior (an int can never reach std::numeric_limits <int>::max () + 5).

3. Consider wrapping the std::vector into a class.

Programs typically use std::vector for many things. Overloading a global operator like you do would make me uncomfortable using your code.

4. Obligatory avoid using namespace std;.

5. Use more meaningful variable names and typenames.

Unless A, R, Q, etc are commonly understood names for special variables in your scientific community, use more meaningful names.

6. Do not omit braces for if-statements or loops.

This is more a subjective style guideline than anything.

\$\endgroup\$
4
  • \$\begingroup\$ These are great comments! Can I ask for clarification? 3,4,5 I get and the points are well taken. #2, is the .size() of something ever not an int? That may be the first time I've heard of this (and hence why I'm on code review). #1 Are you saying that there would be two copies of A that would be made? I'm not sure I understand copy epsilon, even after going over the links. \$\endgroup\$
    – Hooked
    Commented Jan 16, 2015 at 3:55
  • \$\begingroup\$ @Hooked 2) .size() almost always returns an unsigned type, usually something like std::size_t. An int cannot index every possible element of a std::vector. 1) I'll expand my answer with more details. \$\endgroup\$
    – jliv902
    Commented Jan 16, 2015 at 14:41
  • \$\begingroup\$ @Hooked I'm kind of busy right now, so I sort threw something together quickly. If any of it is confusing, please let me know and I'll respond hopefully within the next few days. \$\endgroup\$
    – jliv902
    Commented Jan 16, 2015 at 15:15
  • \$\begingroup\$ No, this additions are great and make sense. Thank you for providing a working example of copy-epsilon for this particular case, it helped make your point. \$\endgroup\$
    – Hooked
    Commented Jan 16, 2015 at 15:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.