Why is this function producing incorrect values? [duplicate]

Question

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.

---

-Edit-

A user has suggested that this may be a duplicate of this Q/A I can not argue against the fact that it does or does not answer my question. At the time of asking this question; I did not know that the bug was coming from the improper use of std::accumulate itself. I wasn't sure if it was coming from the compiler warning that pertained to possible loss of data due to conversion, or if I was casting it wrong, or if it was in how I implemented this function in general. I had already accepted the answer that is found on this page before the link was provided. I will leave this Q/A as is for future reference and readers! Other than that I do appreciate the provided link as it does help to understand where the error was located in my code, what the error was and what was causing it, and how to properly fix it in addition to the accepted answer on this page.

Answer 1

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.