70299 - a==b?c:d is sometimes incorrectly used to mean a==(b?c:d)

Status: RESOLVED WONTFIX

(Core Graveyard :: Tracking, defect)

Description

[timeless]

even the smallest of grammar errors can result in the largest of bugs (take bug 
51237)

------- Additional Comments From Gervase Markham 2001-02-24 03:34 -------
Can someone with better knowledge of regexps than me grep the codebase for 
bracket .* == .* nobracket but question-mark  .* colon .* bracket ?
(see the patch for what I mean.)

If this sort of problem is anywhere else, it's a bug too. And we get to fix 
lots for free, rather than spending six months tracking them down (like this 
one.)
------- Additional Comments From [e-mail] 2001-02-24 17:49 -------
\(.*==[^)?]*\?[^:]*:.*\) is about the best i could find.

but it really needs to check for object typing. a==b?c:d is sometimes meant as 
(a==b)?c:d [correct], or a==(b?c:d). the best way i've found to think about 
them is comparing the type of a to c/d vs. b. unfortunately types come from 
functions.
------- Additional Comments From Jesse Ruderman 2001-02-26 07:05 -------
timeless: or we could just change add parentheses to *all* instances of the 
a == b ? c : d pattern, and not worry about type checking.  Then we'd be 
replacing most instances with (a == b) ? c : d, and a few buggy ones would be 
replaced with a == (b ? c : d).  It's clear from this bug that the prededence 
rules aren't obvious, so adding parentheses in both cases should make code 
clearer.  See also bug 67635, '[rfe] "readability" keyword', which would 
hopefully encourage people to make this type of change.
------- Additional Comments From [e-mail] 2001-02-26 08:56 -------
the type checking is me complaining that i have no good way to recognize 
whether the author meant the former or latter.
------- Additional Comments From Darin Fisher 2001-02-26 11:54 -------
I agree with timeless.  Each case would have to be handled on an individual
basis.  Using a regexp, however, would reveal suspicious expressions, and
using cvs blame we could contact the relevant owners... sound good?
------- Additional Comments From Gervase Markham 2001-02-26 13:24 -------
Created an attachment 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=26214
My attempt using Timeless' regexp on 2001-02-22 source.
------- Additional Comments From Gervase Markham 2001-02-26 13:25 -------
504 hits :-| As timeless says, we really need a grep tool which understands C 
;-)
Anyone volunteering to look at this lot?
------- Additional Comments From Jesse Ruderman 2001-02-26 15:05 -------
Gerv, Darin, Timeless: what do you think of making a new bug for searching for 
and parenthesizing the a==b?c:d pattern, in order to avoid letting this bug 
morph too much?  Please cc me on the new bug if you do :)

Comment 1

[Brendan Eich [:brendan]]

C's expression grammar has some infamous botches, but == binding more tightly
than ? is not one of them.  Still, it's good style to parenthesize such cases
for clarity (or so I learned at the knees of the Unix masters).


GCC always complains about <<, >>, |, &, and ^ used with == and !=, but without
parens (if I recall correctly; the "precplaint", "prec" for precedence).  No
warnings in this case, I take it?

/be

Comment 2

[rbs]

This is a very sneaky one. Along the same disturbing lines, x = a+b?c:d;

Comment 3

[Brendan Eich [:brendan]]

rbs: I don't overparenthesize x = (a ? b : c); because assignment should be
well-known as the next-to-least precedence operator-set in C, C++, JS, etc.
(comma operator being the least).  But extra parens here don't necessarily hurt
readability, as I claim they do in other cases (I've seen ((cast)(&(p->q))) in
non-macro code; it's silly, besides being harder to read than it should be).

/be

Comment 4

[Gervase Markham [:gerv]]

There are 504 unparened cases according to timeless' regexp. Would you like them 
all parenthesised, or merely checked to see if they "appear" that the right 
thing is being done?

(The list is attached to the previous bug.)

Gerv

Comment 5

[timeless]

Attachment: avoid (a==b?c:d) partially moving from a==0?c:b to a?b:c

Comment 6

[timeless]

after looking through the list, it looks like we're ok. gerv's number is off 
(~factor of 2) because grep caught its tail.

there are some whitespace changes and some null changes also. plus a few 
operation reorders.

Comment 7

[Brendan Eich [:brendan]]

timeless: some of your changes are good (whether related to ?: or not), some are
gratuitous, and all must go through module owner r=. Good luck! Passing the bug
around might work best. A diff -wu attachment is necessary to see past all the
tab cleanup.

/be

Comment 8

[Darin Fisher]

r=darin on the netwerk/ changes (although that file is not part of the build).

Comment 9

[timeless]

Attachment: diff -w -u

Comment 10

[Brendan Eich [:brendan]]

Is this another mega-patch that's doomed to lack review and approval because
it's so big, yet so "marginal" in its improvements?  Food for thought, I'm not
sure and I don't mean to be mean.

Take for an example of "marginal" improvement the first change in the patch:

-
	  short percentDefault = (totalCount == 0 ? 0 : ((100 * defaultStruct[i]) /
totalCount));
-
    short percentFromParent = (defaultStruct[i] == 0 ? 0 : ((100 *
setFromParent[i]) / defaultStruct[i]));
+      short percentDefault = (totalCount ? (100 * defaultStruct[i]) /
totalCount : 0);
+      short percentFromParent = (defaultStruct[i] ? (100 * setFromParent[i]) /
defaultStruct[i] : 0);

On the plus side, tabs were eliminated.  Other than that, inverting the sense of
the == 0 test and eliding the != 0 may violate the author's aesthetics -- both
for the implicit test not equal to zero, and for the transposition of the ? and
: parts to put the if-zero case last, perhaps obscuring the if-non-zero case in
the middle of the expression.

On the other hand, the next change seems like a win:

-      PRInt32 selectedIndex = (mSelectedIndex == kNothingSelected ? 0 :
mSelectedIndex+1) % numOptions;
+      PRInt32 selectedIndex = mSelectedIndex == kNothingSelected ? 0 :
(mSelectedIndex+1) % numOptions;

The new version avoids wasting cycles computing 0 % anything.  But why didn't
you parenthesize the condition, to overstate the precedence of == against ?:
like so:

+ PRInt32 selectedIndex = (mSelectedIndex == kNothingSelected) ? 0 :
(mSelectedIndex+1) % numOptions;

This also helps the == expression stand off from the = operator more clearly,
which I view as a readability win.  Otherwise less experienced readers have to
consider whether = or == has higher precedence (again, = has next-to-least
precedence in C-like languages -- but some may wonder, and why sow doubt?).

       PRInt32 startedAtIndex    = selectedIndex;
       PRBool  loopedAround  = PR_FALSE;
-      while ((selectedIndex < startedAtIndex && loopedAround) || !loopedAround) {
+      while ((selectedIndex < startedAtIndex) || !loopedAround) {

This is a nice reduction in logic.  Well done.  The only thing I'd consider is
transposing the clauses, to match the basis case and the order in which the
conditions become true.

I could go on and on, but that's the problem.  Bite-sized patches are much
easier for people to argue about and digest.  Rather than split this bug into a
bunch of new ones, how about a daisy-chain?

/be

Comment 11

[varada]

Dont bother changing anything in the following file
mozilla/profile/Acct/dialshr.cpp
It is obsolete and is being removed.

Comment 12

[edburns]

r=edburns for nsJVMManager.cpp

Comment 13

[Scott MacGregor]

r/sr=mscott for mimemsg.cpp, nsVCardObj and nsMsgCompUtils.cpp

Comment 14

[Gervase Markham [:gerv]]

14 years and 59 days later, I suspect this patch no longer applies. :-(

Gerv