www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 8757] New: Require parenthesization of ternary operator when compounded

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757

           Summary: Require parenthesization of ternary operator when
                    compounded
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: bearophile_hugs eml.cc


--- Comment #0 from bearophile_hugs eml.cc 2012-10-04 09:47:28 PDT ---
In past we have discussed in the D newsgroups about the bug-prone precedence of
the ?: operator. Analysis of shared code repositories (and articles about
static code analysis) shows that this is a common source of bugs. So I suggest
to look for wasy to avoid/reduce such bugs in D code.

One of the possible ideas is (this is a small breaking change): when the ?: is
included in a larger expression, require parentheses around it.

auto x1 = y1 ? z1 : w1; // OK
auto x2 = x0 + (y1 ? z1 : w1); // OK
auto x3 = (x0 + y1) ? z1 : w1; // OK
auto x4 = x0 + y1 ? z1 : w1; // Compilation error
auto x5 = y1 ? z1 : (y2 ? z2 : w2); // OK
auto x6 = y1 ? z1 : y2 ? z2 : w2; // Compilation error


In theory this increases the number of parentheses a little, but in practice in
many similar situations I already put those parentheses, for readability and to
avoid some of my mistakes.

Ideas for other solutions are welcome.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Oct 04 2012
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757



--- Comment #1 from bearophile_hugs 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: -------
Oct 21 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757


Jonathan M Davis <jmdavisProg gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg gmx.com


--- Comment #2 from Jonathan M Davis <jmdavisProg gmx.com> 2013-01-18 20:01:01
PST ---
Any and all operators are bug-prone if you don't understand or remember their
precedence rules. If you want the extra protection against precedence
screw-ups, then use parens. But I don't see any reason to _require_ them. And
honestly, I would be ticked if code like

auto x4 = x0 + y1 ? z1 : w1;

became illegal. I would never use parens here, because I find the precedence
rules in this case to be very clear. This enhancement request is trying to
enforce a particular coding/formatting style, and I'm very much opposed to
that. The compiler shouldn't care how I format my code.

Feel free to use parens to guarantee the correct order of operations if you
don't feel confortable with the precedence rules in a particular expression,
but I don't want that forced on everyone.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 18 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757



--- Comment #3 from bearophile_hugs eml.cc 2013-01-19 00:56:52 PST ---
(In reply to comment #2)

 Any and all operators are bug-prone if you don't understand or remember their
 precedence rules. If you want the extra protection against precedence
 screw-ups, then use parens. But I don't see any reason to _require_ them.



I've shown that it's a common enough bug even for expert C/C++ programmers. So
saying "I don't see any reason" is too much weak. To invalidate this
enhancement request you have to show statistics that confirm your point.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 19 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757



--- Comment #4 from Jonathan M Davis <jmdavisProg gmx.com> 2013-01-19 01:08:50
PST ---
I don't care if it solves half the bugs involving ternary operators that ever
happen. You're suggesting that we force programmers to format their code in a
particular way, and I object to that.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 19 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757



--- Comment #5 from bearophile_hugs eml.cc 2013-01-19 01:28:20 PST ---
(In reply to comment #4)

 I don't care if it solves half the bugs involving ternary operators that ever
 happen.



So you are saying that data in language design should be ignored?



 You're suggesting that we force programmers to format their code in a
 particular way, and I object to that.



C language has some design mistakes, like in its precedence rules. A well
designed language, and one of the design principles of D, has to help the
programmer avoid the most common bugs. D fixes some of the mistakes of C
design. This is a "formatting forced by D", that has saved me few times:


void main() {
    int x, y;
    auto z = x & 1 == y;
}


temp.d(3): Error: 1 == y must be parenthesized when next to operator &


See also issue 5409 for something similar.


You don't want to many parentheses in expressions, but few strategically placed
ones are a small price to pay to save you code from common mistakes.

Your words don't hold water, unless you show that adding a ( ) there harms
readability or some other real coding quality.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 19 2013
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=8757


Dicebot <m.strashun gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |m.strashun gmail.com


--- Comment #6 from Dicebot <m.strashun gmail.com> 2013-01-19 08:30:06 PST ---
It is hard to add something like that because of backward compatibility issues
but I vote for this approaches. Personal formatting preferences mean nothing
compared with possibility to remove common source of bugs. And this one is
really common.

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