Why is this function producing incorrect values?

  • A+

I have a simple function template to calculate the average value of a container:

template<typename T> T array_average( std::vector<T>& values ) {     if( std::is_arithmetic<T>::value ) {         if( !values.empty() ) {             if( values.size() == 1 ) {                 return values[0];             } else {                  return (static_cast<T>( std::accumulate( values.begin(), values.end(), 0 )  ) / static_cast<T>( values.size() ) );             }         } else {             throw std::runtime_error( "Can not take average of an empty container" );          }     } else {         throw std::runtime_error( "T is not of an arithmetic type" );     } } 

I added in the static_cast<>s above to try to force the calculation to the desired type <T>.

When I call this function in main using an uint64_t

std::vector<uint64_t> values{ 1,2,3,4,5,6,7,8,9,10,11,12 }; std::cout << array_average( values ) << '/n'; 

This codes does produce MSVC's compiler warning C4244 possible loss of data due to conversion, but it runs properly and this gives me the expected result and it prints out 6 to the console. This is correct as the actual value is 6.5 but due to the truncation in integer division 6 is correct.

Now If I use the function above with this instead:

std::vector<double> values { 2.0, 3.5, 4.5, 6.7, 8.9 }; std::cout << array_average( values2 ) << '/n'; 

This should give me a result of 5.12 however it is displaying 4.6 instead. This also gives me the same compiler warning as above, but it runs without a runtime error (break in execution) but is giving me incorrect results.

I'm not sure where the bug is in my function. I don't know if this is due to that compiler warning or not, or if it's the way I designed the function itself.


Your static_cast is in the wrong place. You're casting the result of the accumulation, but letting the accumulation run in the type of the initial term (here 0, which is int). So do this instead:

return std::accumulate( values.begin(), values.end(), static_cast<T>(0) ) / static_cast<T>( values.size() ); 

(Note that 4.6 is indeed the result of static_cast<double>(2 + 3 + 4 + 6 + 8) / 5.0).

Comments unrelated to the core of the question:

  • The function should be taking const std::vector<T>&, because it doesn't modify values.
  • If you call the function with a T which is not valid for std::accumulate (e.g. not arithmetic), you will get a compile-time error. The topmost if would have to be if constexpr to work the way you want it to.


:?: :razz: :sad: :evil: :!: :smile: :oops: :grin: :eek: :shock: :???: :cool: :lol: :mad: :twisted: :roll: :wink: :idea: :arrow: :neutral: :cry: :mrgreen: