CODEWITHSUNDEEP
c++c++11method-chainingsequence-points
rr-
rr-

Reputation: 14811

Sequence Points and Method Chaining reloaded

I've read this:

...but I'm still not sure how this should behave:

int should_be_zero
    = stream.seek(4).read_integer()
    - stream.seek(4).read_integer();

Stream::seek() returns *this and seek/read_integer() respectively call fseek/fread on some FILE*.

This should return 0 like this:

  1. stream.seek(4)
  2. stream.read_integer() (at position 4, returns X, stream position advanced to 8)
  3. stream.seek(4)
  4. stream.read_integer() (at position 4, returns Y == X)
  5. X - Y == 0

This worked well for me on gcc, MinGW and MinGW-w64. But when I decided to extend compiler support for MSVC, I discovered that this doesn't work anymore and returns garbage values. Here's what actually happens on MSVC:

  1. stream.seek(4)
  2. stream.seek(4) (again)
  3. stream.read_integer() (at position 4, returns X, stream position advanced to 8)
  4. stream.read_integer() (at position 8, returns Y != X)
  5. X - Y != 0

Is such execution order well defined? If not, how can I protect myself against shooting myself in the foot like this in the future?

(Wrapping the calls with brackets doesn't seem to do anything.)

Upvotes: 3

Views: 116

Answers (2)

Mats Petersson
Mats Petersson

Reputation: 129344

The internal order of execution within an expression is not defined. Only the obvious behavior of operator precedence is defined.

So, in this case, the compiler is obliged to call stream.seek(4) twice [unless the compiler figures out that it's "the same result either way"] and stream.read_integer() twice. But the order of those calls is undetermined (or whatever the term is in the C++ standard) - in other words, the compiler can order those four calls any way it likes.

Your code would be even more risky if you did something like:

 int x
    = stream.seek(4).read_integer()
    - stream.read_integer();

since it's not well defined which of the two reads happen in which order now - it could call the second read_integer first (at offset 0) or after the seek and read at offset 8. Nobody knows which, and the compiler may even re-arrange them if you make subtle changes to the code (e.g. it decides to do things in a diffferent order because you added another variable that uses another register -> re-arrange code to use registers better...)

The solution is to introduce intermediate variables:

int a = stream.seek(4).read_integer();
int b = stream.seek(4).read_integer();

int should_be_zero = a - b; // Or b - a, if that's what you want... :)

This should be done in every piece of code where the order of execution is important for the correctness of the code - and bear in mind that "side-effects" (such as reading input, writing output, modifying state) are definitely dependent on order of execution.

Upvotes: 2

Revolver_Ocelot
Revolver_Ocelot

Reputation: 8785

It seems like this behaviour is allowed:

1.9/15
Except where noted, evaluations of operands of individual operators and of subexpressions of individual expressions are unsequenced. [ Note: In an expression that is evaluated more than once during the execution of a program, unsequenced and indeterminately sequenced evaluations of its subexpressions need not be performed consistently in different evaluations. —end note ] The value computations of the operands of an operator are sequenced before the value computation of the result of the operator.
[...]
When calling a function (whether or not the function is inline), every value computation and side effect associated with any argument expression, or with the postfix expression designating the called function, is sequenced before execution of every expression or statement in the body of the called function. [ Note: Value computations and side effects associated with different argument expressions are unsequenced. —end note ]

Unsequenced means "evaluation of left hand side and right hand side can be interleaved" like "evaluate part of left expression, evaluate right expression, evaluate rest of left expression"

Upvotes: 1

Related Questions