[Issue 8757] Require parenthesization of ternary operator when compounded

[d-bugmail at puremagic.com]

http://d.puremagic.com/issues/show_bug.cgi?id=8757



--- Comment #1 from bearophile_hugs at eml.cc 2012-10-21 10:22:51 PDT ---
From: http://www.viva64.com/en/examples-V502/

Some bugs caused by ternary operator usage in already tested and used code of
professionally-managed projects.

-----------------

Grid Control Re-dux

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '|' operator.


void CGridCtrlDemoDlg::UpdateMenuUI()
{
  ...
  GetMenu()->CheckMenuItem(IDC_HORZ_LINES, MF_BYCOMMAND |
    (bHorzLines)? MF_CHECKED: MF_UNCHECKED);
  GetMenu()->CheckMenuItem(IDC_LISTMODE, MF_BYCOMMAND |
    (m_Grid.GetListMode())? MF_CHECKED: MF_UNCHECKED);
  GetMenu()->CheckMenuItem(IDC_VERT_LINES, MF_BYCOMMAND |
    (bVertLines)? MF_CHECKED: MF_UNCHECKED);
  GetMenu()->EnableMenuItem(IDC_SINGLESELMODE, MF_BYCOMMAND |
    (m_Grid.GetListMode())? MF_ENABLED: MF_DISABLED|MF_GRAYED);
  .....
  GetMenu()->CheckMenuItem(ID_HIDE2NDROWCOLUMN,
    MF_BYCOMMAND |
    (m_bRow2Col2Hidden)? MF_CHECKED: MF_UNCHECKED);
  ...
}

This code is incorrect as the priority of '?:' operator is lower than of '|'.
The program works correctly because of MF_BYCOMMAND == 0. Nonetheless this code
is potentially dangerous.

-----------------

FCEUX

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '|' operator. fceux
memwatch.cpp 711


static BOOL CALLBACK MemWatchCallB(....)
{
  ...
  EnableMenuItem(memwmenu, MEMW_FILE_SAVE,
    MF_BYCOMMAND | fileChanged ? MF_ENABLED:MF_GRAYED);
  ...
}

It works because of sheer luck, since #define MF_BYCOMMAND 0x00000000L.

-----------------

IPP Samples

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '|' operator. vm vm_file_win.c
393


vm_file* vm_file_fopen(....)
{
  ...
  mds[3] = FILE_ATTRIBUTE_NORMAL |
           (islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING;
  ...
}

0 is always written into mds[3]. Parentheses should be used: mds[3] =
FILE_ATTRIBUTE_NORMAL | ((islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING).

-----------------

Newton Game Dynamics

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '*' operator. physics
dgminkowskiconv.cpp 1061


dgInt32 CalculateConvexShapeIntersection (....)
{
  ...
  den = dgFloat32 (1.0e-24f) *
        (den > dgFloat32 (0.0f)) ?
          dgFloat32 (1.0f) : dgFloat32 (-1.0f);
  ...
}

Identical errors can be found in some other places:

    V502 Perhaps the '?:' operator works in a different way than it was
expected. The '?:' operator has a lower priority than the '*' operator. physics
dgminkowskiconv.cpp 1081

-----------------

Chromium

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '-' operator. views
custom_frame_view.cc 400


static const int kClientEdgeThickness;

int height() const;

bool ShouldShowClientEdge() const;

void CustomFrameView::PaintMaximizedFrameBorder(
  gfx::Canvas* canvas)
{
  ...
  int edge_height = titlebar_bottom->height() -
    ShouldShowClientEdge() ? kClientEdgeThickness : 0;
  ...
}

-----------------

OTS

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '+' operator. ots gdef.cc 278


bool version_2;

bool ots_gdef_parse(....)
{
  ...
  const unsigned gdef_header_end = static_cast<unsigned>(8) +
    gdef->version_2 ? static_cast<unsigned>(2) :
                      static_cast<unsigned>(0);
  ...
}

-----------------

ReactOS

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '+' operator. uniata id_dma.cpp
1610


VOID NTAPI
AtapiDmaInit(....)
{
  ...
  ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7;
  ...
}

The "0x54 + (dev < 3)" condition is always true. This is the correct code:
ULONG treg = 0x54 + ((dev < 3) ? (dev << 1) : 7).

-----------------

Chromium

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '+' operator. rtp_rtcp
rtp_receiver_video.cc 480


WebRtc_Word32
RTPReceiverVideo::ReceiveH263Codec(....)
{
  ...
  if (IP_PACKET_SIZE < parsedPacket.info.H263.dataLength +
       parsedPacket.info.H263.insert2byteStartCode ? 2:0)
  ...
}

Parentheses should be used.

Identical errors can be found in some other places:

    V502 Perhaps the '?:' operator works in a different way than it was
expected. The '?:' operator has a lower priority than the '+' operator.
rtp_rtcp rtp_receiver_video.cc 504

-----------------

MongoDB

V502 Perhaps the '?:' operator works in a different way than it was expected.
The '?:' operator has a lower priority than the '<<' operator. version.cpp 107


string sysInfo() {
  ....
  stringstream ss;
  ....
  ss << (sizeof(char *) == 8) ? " 64bit" : " 32bit";
  ....
}

A very nice sample. 0 or 1 will be printed instead of "32bit"/"64bit".

-----------------

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------