HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Multiply vector elements by a scalar value using STL and templates

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stlelementsscalarmultiplyvaluetemplatesusingandvector

Problem

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 
vector  operator*(const Q c, const vector &A) {
  vector  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 
vector  operator*(const Q c, const vector &A) {

  vector  R(A.size());
  std::transform(A.begin(), A.end(), R.begin(),
                 std::bind1st(std::multiplies(),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.

Solution

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  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 
std::vector  operator* (const Q c, std::vector  A)
{
    std::transform (A.begin (), A.end (), A.begin (),
                 std::bind1st (std::multiplies  () , c)) ;
    return A ;
}


  1. 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 ::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 ::max () + 5 ;
std::vector  v1 (size) ;

for (int n = std::numeric_limits ::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 ::max () + 5).

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

  1. Obligatory avoid using namespace std;.



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

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



This is more a subjective style guideline than anything.

Code Snippets

std::vector <int> v1 = // ... insert elements
auto v2 = 3 * v1 ;
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 ;
}
typedef std::vector <T>::size_type size_type ;
for (size_type i = 0; i < A.size (); ++i)
using size_type = decltype (A.size ()) ;
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' ;
}

Context

StackExchange Code Review Q#77546, answer score: 5

Revisions (0)

No revisions yet.