got an unexpected answer from the x?y:z expression

Question

Here is a simple C++ snippet:

int x1 = 10, x2 = 20, y1 = 132, y2 = 12, minx, miny, maxx, maxy;
x1 <= x2 ? minx = x1, maxx = x2 : minx = x2, maxx = x1;
y1 <= y2 ? miny = y1, maxy = y2 : miny = y2, maxy = y1;
cout << "minx=" << minx << "\n";
cout << "maxx=" << maxx << "\n";
cout << "miny=" << miny << "\n";
cout <<" maxy=" << maxy << "\n";

I thought the result should be:

minx=10
maxx=20
miny=12
maxy=132

But actually the result is:

minx=10
maxx=10
miny=12
maxy=132

Could someone give an explanation why maxx is not 20? Thanks.

Yet another reason to "not try to be clever" with conditional expressions and use `if` instead. The compiler will do the same thing either way [assuming you add the relevant parenthesis so that it does what you actually wanted]. Ternary expressions may be useful sometimes, but this is a good example of what NOT to do with them. — Mats Petersson, May 18 '13 at 21:44
When I first saw the title of this question I thought it was [going to be about PHP](http://phpsadness.com/sad/30). — Ray Toal, May 19 '13 at 06:01
I always found it easy to remember that in most languages the comma is at the bottom, the very bottom of the order of operations list. So this code actually looks "obviously wrong" to me. You don't really want to sprinkle parens **everywhere**. — Steven Lu, Jun 06 '13 at 04:37

score 40 · Answer 1 · edited May 23 '17 at 11:43

40

Due to operator precedence, expression is parsed like this:

(x1<=x2 ? minx=x1,maxx=x2 : minx=x2), maxx=x1;

you can solve this with:

(x1<=x2) ? (minx=x1,maxx=x2) : (minx=x2, maxx=x1);

And actually you don't need the first two pairs of parentheses. Also check this question.

edited May 23 '17 at 11:43

Community

1
1

answered May 18 '13 at 21:38

perreal

94,503
21
155
181

Daniel Fischer · Answer 2 · 2013-05-19T00:00:47.983

The precedence of the conditional operator is greater than that of the comma operator, so

x1<=x2 ? minx=x1,maxx=x2 : minx=x2,maxx=x1;

is parenthesised as

(x1<=x2 ? minx=x1,maxx=x2 : minx=x2),maxx=x1;

Thus the last assignment is done regardless of the condition.

To fix it, you can

use parentheses:
```
x1 <= x2 ? (minx = x1, maxx = x2) : (minx = x2, maxx = x1);
```
(you don't need the parentheses in the true branch, but it's IMO better to have them too).

use two conditionals:

minx = x1 <= x2 ? x1 : x2;
maxx = x1 <= x2 ? x2 : x1;

use an if:

if (x1 <= x2) {
    minx = x1;
    maxx = x2;
} else {
    minx = x2;
    maxx = x1;
}

Compiled with or without optimisations, the if version and the parenthesised single conditional with commas produce the same assembly both under gcc (4.7.2) and clang (3.2), it's reasonable to expect that from other compilers too. The version with the two conditionals produces different assembly, but with optimisations, both these compilers emit only one cmp instruction for that too.

In my view, the if version is the easiest to verify the correctness of, so preferable.

+1 for testing the difference in generated code and providing a readability argument. — fluffy, May 19 '13 at 06:14

Mats Petersson · Answer 3 · 2013-05-18T22:39:30.463

Whilst others have explained what the cause of the problem is, I think the "better" solution should be to write the conditional with if:

int x1 = 10, x2=20, y1=132, y2=12, minx, miny, maxx, maxy;
if (x1<=x2) 
{ 
   minx=x1;
   maxx=x2;
}
else
{
   minx=x2; 
   maxx=x1;
}
if (y1<=y2)
{
    miny=y1;
    maxy=y2;
} 
else 
{
    miny=y2;
    maxy=y1;
}

Yes, it's several lines longer, but it's also easier to read and clear exactly what goes on (and if you need to step through it in the debugger, you can easily see which way it goes).

Any modern compiler should be able to convert either of these to fairly efficient conditional assignments that does a good job of avoiding branches (and thus "bad branch prediction").

I prepared a little test, which I compiled using

g++ -O2 -fno-inline -S -Wall ifs.cpp

Here's the source (I had to make it parameters to ensure the compiler didn't just calculate the correct value directly and just do mov $12,%rdx, but actually did a compare and decide with is bigger):

void mine(int x1, int x2, int y1, int y2)
{
    int minx, miny, maxx, maxy;
    if (x1<=x2) 
    { 
    minx=x1;
    maxx=x2;
    }
    else
    {
    minx=x2; 
    maxx=x1;
    }
    if (y1<=y2)
    {
    miny=y1;
    maxy=y2;
    } 
    else 
    {
    miny=y2;
    maxy=y1;
    }

    cout<<"minx="<<minx<<"\n";
    cout<<"maxx="<<maxx<<"\n";
    cout<<"miny="<<miny<<"\n";
    cout<<"maxy="<<maxy<<"\n";
}

void original(int x1, int x2, int y1, int y2)
{
    int minx, miny, maxx, maxy;
    x1<=x2 ? (minx=x1,maxx=x2) : (minx=x2,maxx=x1);
    y1<=y2 ? (miny=y1,maxy=y2) : (miny=y2,maxy=y1);
    cout<<"minx="<<minx<<"\n";
    cout<<"maxx="<<maxx<<"\n";
    cout<<"miny="<<miny<<"\n";
    cout<<"maxy="<<maxy<<"\n";
}

void romano(int x1, int x2, int y1, int y2)
{
    int  minx, miny, maxx, maxy;

    minx = ((x1 <= x2) ? x1 : x2);
    maxx = ((x1 <= x2) ? x2 : x1);
    miny = ((y1 <= y2) ? y1 : y2);
    maxy = ((y1 <= y2) ? y2 : y1);
    cout<<"minx="<<minx<<"\n";
    cout<<"maxx="<<maxx<<"\n";
    cout<<"miny="<<miny<<"\n";
    cout<<"maxy="<<maxy<<"\n";
}

int main()
{
    int x1=10, x2=20, y1=132, y2=12;
    mine(x1, x2, y1, y2);
    original(x1, x2, y1, y2);
    romano(x1, x2, y1, y2);
    return 0;
}

The generated code looks like this:

_Z4mineiiii:
.LFB966:
    .cfi_startproc
    movq    %rbx, -32(%rsp)
    movq    %rbp, -24(%rsp)
    movl    %ecx, %ebx
    movq    %r12, -16(%rsp)
    movq    %r13, -8(%rsp)
    movl    %esi, %r12d
    subq    $40, %rsp
    movl    %edi, %r13d
    cmpl    %esi, %edi
    movl    %edx, %ebp
    cmovg   %edi, %r12d
    cmovg   %esi, %r13d
    movl    $_ZSt4cout, %edi
    cmpl    %ecx, %edx
    movl    $.LC0, %esi
    cmovg   %edx, %ebx
    cmovg   %ecx, %ebp
        .... removed actual printout code that is quite long and unwieldy... 
_Z8originaliiii:
    movq    %rbx, -32(%rsp)
    movq    %rbp, -24(%rsp)
    movl    %ecx, %ebx
    movq    %r12, -16(%rsp)
    movq    %r13, -8(%rsp)
    movl    %esi, %r12d
    subq    $40, %rsp
    movl    %edi, %r13d
    cmpl    %esi, %edi
    movl    %edx, %ebp
    cmovg   %edi, %r12d
    cmovg   %esi, %r13d
movl    $_ZSt4cout, %edi
cmpl    %ecx, %edx
movl    $.LC0, %esi
cmovg   %edx, %ebx
cmovg   %ecx, %ebp
        ... print code goes here ... 
_Z6romanoiiii:
    movq    %rbx, -32(%rsp)
    movq    %rbp, -24(%rsp)
    movl    %edx, %ebx
    movq    %r12, -16(%rsp)
    movq    %r13, -8(%rsp)
    movl    %edi, %r12d
    subq    $40, %rsp
    movl    %esi, %r13d
    cmpl    %esi, %edi
    movl    %ecx, %ebp
    cmovle  %edi, %r13d
    cmovle  %esi, %r12d
movl    $_ZSt4cout, %edi
cmpl    %ecx, %edx
movl    $.LC0, %esi
cmovle  %edx, %ebp
cmovle  %ecx, %ebx
        ... printout code here....

As you can see, mine and original is identical, and romano uses slightly different registers and a different form of cmov, but otherwise they do the same thing in the same number of instructions.

+1. The code posted in the question is exactly the kind of code that makes many organizations simply ban the ternary operator outright. The ternary operator has its place, but the posted use isn't that place. — David Hammen, May 18 '13 at 22:01
Re *Yes, it's several lines longer* -- It wouldn't be quite so bad had you used 1TBS. :) — David Hammen, May 18 '13 at 22:04
@DavidHammen Yes, you can style it some other way [this is just how I usually write my own code], but probably not (without making it quite messy) get it down to 2 lines... Even four reasonably readable lines is pushing it. So the statement is still "several lines longer". And my point was to make it more readable, not something suitable for an IOCCC entry. — Mats Petersson, May 18 '13 at 22:11
+1. Conditional operator did not look good here. I posted another variant for this with 'short' conditional operators which keeps code as readable/compact as it could be. Interesting thing here is how machine code will be generated. — Roman Nikitchenko, May 18 '13 at 22:12
One advantage of the conditional operator is that it makes the compiler much more likely to use the `cmov` operations which remove potential pipeline bubbles from the code. — Zan Lynx, May 18 '13 at 22:23
@ZanLynx: Most MODERN compilers understand that using branches is a bad idea (and that is usually not applicable when you use side-effects inside the branches of a ternary operator, but when you do something like `biggest = (a > b)?a:b;`). See my edit for gcc results. — Mats Petersson, May 18 '13 at 22:39
Well, guys, you think only about Intel platform... ARM or PowerPC situation could be slightly different. Thought for most cases it's only question of curiosity - if you really need to save 1 instruction - code it using embedded asm. — Roman Nikitchenko, May 18 '13 at 22:46
I have no reason to believe that a good compiler (e.g. `armcc` or `gcc` on ARM should do worse than the x86 variant - the understanding of "conditional instructions" should be there in both of those compilers). I haven't used any PowerPC compilers, but from what I understand the Diab compiler that used to be popular when I had SOME contact with PowerPC (through colleagues working with it), it was pretty clever at that time - I expect it hasn't got worse. — Mats Petersson, May 18 '13 at 22:49
@Mats I had couple of crazy projects with Diab and I had experience of migration of whole codebase by user request from GCC to Diab and from Diab to GCC. My understanding is Diab is much worse than GCC. At least in reliability and quality of static code analysis. Probably GCC simply went better faster than Diab. Main problem is when you critically need performance standard solutions could not bring what you expect. :-) — Roman Nikitchenko, May 19 '13 at 04:32

Roman Nikitchenko · Answer 4 · 2013-05-18T22:08:09.477

Interesting question both about operations precedence and code generation.

OK, , operation has VERY low priority (lowest, see reference table). Due to this fact your code is the same like the following lines:

((x1<=x2) ? minx=x1,maxx=x2 : minx=x2),maxx=x1;
((y1<=y2) ? miny=y1,maxy=y2 : miny=y2),maxy=y1;

Actually only C/C++ grammar prevents first , from the same behaviour.

Yet another really dangerous place in C/C++ operations precedence is bitwise operations and comparison. Consider about the following fragment:

int a = 2;
int b = (a == 2|1); // Looks like check for expression result? Nope, results 1!

Looking forward, I'd recommend rewriting your fragment in this way keeping balance between efficiency and readability:

minx = ((x1 <= x2) ? x1 : x2);
maxx = ((x1 <= x2) ? x2 : x1);
miny = ((y1 <= y2) ? y1 : y2);
maxy = ((y1 <= y2) ? y2 : y1);

Most interesting fact about this fragment of code is such style could result most effective code for some architectures like ARM because of condition bit flags in CPU instruction set (condition duplication costs nothing and more, points compiler to right code fragments).

score 1 · Answer 5 · answered May 18 '13 at 21:42

Because of the operator precedence :

(x1<=x2 ? minx=x1,maxx=x2 : minx=x2),maxx=x1

You can fix it with :

int x1=10, x2=20, y1=132, y2=12, minx, miny, maxx, maxy;
x1<=x2 ? (minx=x1,maxx=x2) : (minx=x2,maxx=x1);
y1<=y2 ? (miny=y1,maxy=y2) : (miny=y2,maxy=y1);
cout<<"minx="<<minx<<"\n";
cout<<"maxx="<<maxx<<"\n";
cout<<"miny="<<miny<<"\n";
cout<<"maxy="<<maxy<<"\n";

TemplateRex · Answer 6 · 2013-05-19T11:31:30.890

In C++11 you can use std::tie and std::make_pair to make this obviously-correct-at-a-glance (TM)

#include <tuple>
#include <utility>
#include <iostream>

using namespace std;

int main()
{
    int x1 = 10, x2=20, y1=132, y2=12, minx, miny, maxx, maxy;

    tie(minx, maxx) = (x1 <= x2)? make_pair(x1, x2) : make_pair(x2, x1);
    tie(miny, maxy) = (y1 <= y2)? make_pair(y1, y2) : make_pair(y2, y1);

    cout<<"minx="<<minx<<"\n";
    cout<<"maxx="<<maxx<<"\n";
    cout<<"miny="<<miny<<"\n";
    cout<<"maxy="<<maxy<<"\n";
}

Online output.

This is semantically equivalent to all the other solutions posted, and with any decent optimizing compiler, has no overhead whatsoever. It is syntactically much nicer because it has

a minimum of code repetition,
the 4 assigned-to-variables are all on the left hand side of the assignment, and
the 4 assigned-from variables are all on the right.

As a slight variation that generalizes to finding pointers to the min and max element of sequences, you could use std::minmax_element and the fact that raw arrays have non-member functions begin() and end() (both C++11 features)

#include <algorithm>
#include <tuple>
#include <iostream>

using namespace std;

int main()
{
    int x[] = { 10, 20 }, y[] = { 132, 12 }, *minx, *miny, *maxx, *maxy;

    tie(minx, maxx) = minmax_element(begin(x), end(x));
    tie(miny, maxy) = minmax_element(begin(y), end(y));

    cout<<"minx="<<*minx<<"\n";
    cout<<"maxx="<<*maxx<<"\n";
    cout<<"miny="<<*miny<<"\n";
    cout<<"maxy="<<*maxy<<"\n";
}

Online output.

got an unexpected answer from the x?y:z expression

6 Answers6