Why incorrect code works

This bug was found in Miranda NG’s project. The code contains an error that analyzer diagnoses in the following way: V502 Perhaps the ‘?:’ operator works in a different way than was expected. The ‘?:’ operator has a lower priority than the ‘|’ operator..

#define MF_BYCOMMAND 0x00000000L
void CMenuBar::updateState(const HMENU hMenu) const
{
  ....
  ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
    MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED);
  ....
}

Explanation

Sometimes we see that totally incorrect code happens, against all odds, to work just fine! Now, for experienced programmers this really comes as no surprise (another story), but for those that have recently started learning C/C++, well, it might be a little baffling. So today, we’ll have a look at just such an example.

In the code shown above, we need to call CheckMenuItem() with certain flags set; and, on first glance we see that if bShowAvatar is true, then we need to bitwise OR MF_BYCOMMAND with MF_CHECKED – and conversely, with MF_UNCHECKED if it’s false. Simple!

In the code above the programmers have chosen the very natural ternary operator to express this (the operator is a convenient short version of if-then-else):

MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED

The thing is that the priority of |operator is higher than of ?: operator. (see Operation priorities in C/C++). As a result, there are two errors at once.

The first error is that the condition has changed. It is no longer – as one might read it – “dat->bShowAvatar”, but “MF_BYCOMMAND | dat->bShowAvatar”.

The second error – only one flag gets chosen – either MF_CHECKED or MF_UNCHECKED. The flag MF_BYCOMMAND is lost.

But despite these errors the code works correctly! Reason – sheer stroke of luck. The programmer was just lucky that the MF_BYCOMMAND flag is equal to 0x00000000L. As the MF_BYCOMMAND flag is equal to 0, then it doesn’t affect the code in any way. Probably some experienced programmers have already gotten the idea, but I’ll still give some comments in case there are beginners here.

First let’s have a look at a correct expression with additional parenthesis:

MF_BYCOMMAND | (dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED)

Replace macros with numeric values:

0x00000000L | (dat->bShowAvatar ? 0x00000008L : 0x00000000L)

If one of the operator operands | is 0, then we can simplify the expression:

dat->bShowAvatar ? 0x00000008L : 0x00000000L

Now let’s have a closer look at an incorrect code variant:

MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED

Replace macros with numeric values:

0x00000000L | dat->bShowAvatar ? 0x00000008L : 0x00000000L

In the subexpression “0x00000000L | dat->bShowAvatar” one of the operator operands | is 0. Let’s simplify the expression:

dat->bShowAvatar ? 0x00000008L : 0x00000000L

As a result we have the same expression, this is why the erroneous code works correctly; another programming miracle has occurred.

16k4ri

Correct code

There are various ways to correct the code. One of them is to add parentheses, another – to add an intermediate variable. A good old if operator could also be of help here:

if (dat->bShowAvatar)
  ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR, 
                  MF_BYCOMMAND | MF_CHECKED);
else
  ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
                  MF_BYCOMMAND | MF_UNCHECKED);

We really don’t insist on using this exact way to correct the code. It might be easier to read it, but it’s slightly lengthy, so it’s more a matter of preferences.

Recommendation

Our recommendation is simple – try to avoid complex expressions, especially with ternary operators. Also don’t forget about parentheses.

The ?: is very dangerous. Sometimes it just slips your mind that it has a very low priority and it’s easy to write an incorrect expression. People tend to use it when they want to clog up a string, so try not to do that.

Written by Andrey Karpov.
This error was found with PVS-Studio static analysis tool.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s