[cfe-dev] [PATCH] -Wconversion-null

[Douglas Gregor]

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 at 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