Strange warning of front increment operator

Question

I have implemented an "incremental mean" (I am posing just the .cpp file)

#include "../hpp/CIncrementalMean.hpp"

IncrementalMean::IncrementalMean() : m_mean(0.0), m_cntr(0) {}

IncrementalMean::~IncrementalMean() {}

void IncrementalMean::addValueToMean(double valIn)
{
  m_mean = (m_cntr * m_mean + valIn) / ++m_cntr;
}

int IncrementalMean::getCounter() const
{
  return m_cntr;
}

double IncrementalMean::getMean() const
{
  return m_mean;
}

And when I build it (gcc4.9 Ubuntu14.04), I get the following warning:

warning: operation on ‘((IncrementalMean*)this)->IncrementalMean::m_cntr’ may be undefined [-Wsequence-point]

I am not sure why it is displayed. Can someone give me an advise? Shall I do

m_mean = (m_cntr * m_mean + valIn) / (m_cntr + 1);
m_cntr++;

instead?

But anyway, can someone explain me the warning?

http://stackoverflow.com/questions/949433/why-are-these-constructs-undefined-behavior ; http://stackoverflow.com/questions/4176328/undefined-behavior-and-sequence-points — Mat, May 23 '14 at 09:24
You are lucky that you got warning. Now fix it as later code you mentioned. — Mohit Jain, May 23 '14 at 09:26

score 4 · Accepted Answer · edited Apr 13 '17 at 12:19

4

m_mean = (m_cntr * m_mean + valIn) / ++m_cntr;
        ^^^^^^^^^                   ^^^^^^^^^^
         READING                     WRITING

You are reading and writing a variable without a sequence point between the operations. This can be rearranged by your compiler and thus it is Undefined Behaviour :D

EDIT: complementing a little bit, you'll have less floating point error accumulation if you use:

m_mean += (valIn - m_mean) / ++m_cntr;

(see here) AND you'll avoid the Undefined Behaviour.

edited Apr 13 '17 at 12:19

Community

1
1

answered May 23 '14 at 09:23

Massa

8,647
2
25
26

1

Then, I shall slit it in two lines: `m_mean = (m_cntr * m_mean + valIn); m_mean /= ++m_cntr;` – thedarkside ofthemoon May 23 '14 at 09:31

fredoverflow · Answer 2 · 2014-05-23T09:31:19.757

3

Let us simplify your code to identify the problem:

a = 0;
b = a / ++a;

What division is performed exactly in line 2? You are probably thinking that it must be:

b = 0 / 1;

When in fact the compiler is free to evaluate ++a first and a second, giving you:

b = 1 / 1;

So what does the code do? The technical term is "undefined behavior", but all you need to remember is "don't increment a variable and use it again in the same expression".

Regarding your design, it would probably make much more sense to have two data members counter and sum and defer the division to a member function called calculateMean or something. Then you don't have to multiply and divide (I smell precision problems!) every time addValue is called.

edited May 23 '14 at 09:31

answered May 23 '14 at 09:26

fredoverflow

256,549
94
388
662

And If the values are big and many... it may overflows... How big is the difference (in cycles)? – thedarkside ofthemoon May 23 '14 at 09:39
Actually, incremental updating off the mean introduces **less** errors than calculating it all in the end. – Massa May 23 '14 at 10:02
@Massa Let's say we want to calculate the mean of three numbers a, b and c. Are you seriously saying that `(a + b + c) / 3` is going to be less precise than `(((((0 * 0 + a) / 1) * 1 + b) / 2) * 2 + c) / 3`? – fredoverflow May 23 '14 at 13:08
@FredOverflow now let's say you want to calculate the mean of three billion numbers... Read [this](http://math.stackexchange.com/questions/106700/incremental-averageing) and you are going to understand... – Massa May 23 '14 at 13:17
1

@Massa The accepted answer has two formulas, one of them with precision problems, which is exactly the one OP has provided. Note in my example how each division by x is immediately cancelled out with a multiply by x; this obviously cannot be good for precision. – fredoverflow May 23 '14 at 13:21
That's why I brought the other formula (`m_mean += (valIn - m_mean) / ++m_cntr`) to attention in my edit :D But you are absolutely right WRT "cannot be good for precision"... – Massa May 23 '14 at 13:24
@Massa Very nice. I hereby retract my objections ;) – fredoverflow May 23 '14 at 13:27

score 2 · Answer 3 · answered May 23 '14 at 09:23

The GNU compiler collection (GCC) since 4.6.3 warns when the sequence order of operations is ambiguous.

Since you are modifying m_cntr in denominator and using this value in calculations in numerator, it is not obvious what will be done (i.e. which part of this fraction will be calculated earlier). So, it is better to avoid such constructions.

Strange warning of front increment operator

3 Answers3