[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-dev
Subject:    Re: [cfe-dev] [PATCH] -Wconversion-null
From:       Douglas Gregor <dgregor () apple ! com>
Date:       2012-03-20 5:23:02
Message-ID: 4C4AB724-175A-48F3-AB3F-80BCC5077511 () apple ! com
[Download RAW message or body]

Not such if you meant to CC the list or not, but I'll bring it back into the \
discussion.

On Mar 18, 2012, at 10:44 AM, James K. Lowden wrote:

> On Sat, 17 Mar 2012 20:52:59 -0700
> Douglas Gregor <dgregor@apple.com> wrote:
> 
> I appreciate your effort to educate me.  "Any fool can learn from his
> own experience; a wise man learns from others'."  
> 
> > Turning these warnings on in our codebase yielded a significant
> > number of real bugs with very few false positives. 
> 
> Would "very few" false positives be, say, less than 1%?  Are you then
> saying 99% of uses that lacked parenthesis did so incorrectly?
> IOW most cases of 
> 
> 	a && b || c
> 
> should have been 
> 
> 	a && (b || c)
> 
> ?
> 
> I would expect the ratio to be the reverse: 99% redundant parenthesis
> and 1% error.  Or so.  But if you have hard data or could point me to
> published results, I'm interested. I'm not so in love with my opinion
> that I can't change it.  

I can't give hard data from a proprietary code base. What I can say is that at least \
two major corporations vet Clang's warnings across their entire code bases when they \
go into Clang, and we tweak said warnings until the false positive rate is acceptable \
(or jettison the warning if it can't be made to be acceptable). We're strongly \
user-focused here, and we tune our diagnostics to what works out in the real world.

> Of course, denominators matter.  If the code base of which you're
> speaking had previously had been reviewed and had redundant parenthesis
> added to reduce the warning count to zero, and then was checked again at
> some later date, I might expect a somewhat higher hit ratio.  But then
> we'd be excluding redundant parenthesis from the false-positive count.  

At least one of the large codebases referenced above had never been run through a \
compiler with a "real" parentheses warning.

> I also recognize the value of verification.  I've seen what e.g.
> Coverity can do, and I've read papers about the value of even partial
> verification. I'm all for it.  (As usual, Dijkstra was right.)

Verification is out of scope for the compiler proper, of course.

> What I object to is the idea that "(a && b) || c" should become "best
> practice", that we should expect "good code" to use extra parenthesis
> just in case.    That conditional logic should be checked, yes.  That
> what's been said already needs to be reinforced with extra parentheis,
> no.  

One could say that this ship has already sailed, given GCC's warnings about \
parentheses.

> Unfortunately, as things stand, the only evidence that the code has
> been checked is the presence of extra parenthesis.  I can think of
> better ways, as I'm sure you can, too.  A database of verified tests,
> for instance, would be more useful and not in the least harmful.  

If there's a better way to find these issues within the constraints of the compiler, \
that would be very interesting.

> > They exist, and I've returned their bug reports with an explanation
> > of the precedence rules. Clang helped them already. 
> 
> Interesting.  So the warning didn't help them at all!   They got the
> logic wrong, wrote it wrong, tested it wrong (or not) and chose not to
> use or not to mind the warning.

They had a simple test that would only have ended up wrong in a failure case. It was \
unlikely they would hit such a case in normal testing. 

> Instead, the warning helped *you* help them.  It pointed out instances
> of reliance on Boolean logic precedence.  (That doesn't make the
> warning is pointless.  It does support my point that Clang alone can't
> help the helpless.)    

The warning pointed out an existing bug in the user's code. That the programmer \
assumed it was a compiler bug rather than his own misunderstanding of precedence \
rules proves nothing except that at least some programmers *haven't* fully \
internalized the precedence rules (or are dealing with code complex enough that they \
aren't clear).

> > Code gets refactored, and bugs get introduced. 
> 
> Granted.  Can you explain to me, though, how warning about parenthesis
> protects against that?  If "a && b || c" is correct, "(a && b) || c" is
> correct.  If not, not.  If refactoring changes the correctness, either
> form becomes equally wrong.  I'm afraid I don't see any benefit.  

Parts of expressions get copied around as conditions get more complicated, functions \
get manually "inlined", etc. The indentation may even make it look like one has the \
right precedence when it is, indeed, wrong.

> I have no wish to argue only to hear the sound of my own voice (as it
> were).  I am trying to challenge what I see as the conventional wisdom
> that Boolean logical operator precedence is problematic.  I find it
> hard to separate that line of thinking from smug egotism on the part of
> those writing the warnings.  And I see nothing at all cavalier or
> unsafe in depending on the compiler or the *reader* to understand the
> intent of Boolean tests in the code.  

Programmers, even good ones, make mistakes, and one of the roles of the compiler is \
to catch those classes of mistakes it can and alert the user to the problem before it \
manifests. Conventional wisdom holds that Boolean logical operator precedence is \
problematic, and I've seen that borne out in the code bases I work with. On purely \
philosophical grounds, or if I viewed this warning solely as something that may or \
not help me as a programmer, I would probably tend to agree with you. But for me, \
data is king, and the data I've seen supports the warning for the vast majority of \
users.

	- Doug

_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


[prev in list] [next in list] [prev in thread] [next in thread]