On Thu, 26 Jul 2018 at 13:22, Arthur O'Dwyer via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> On Thu, Jul 26, 2018 at 12:52 PM, Richard Smith <rich...@metafoo.co.uk> > wrote: > >> Other than the (fixed) ffmpeg false positive, I see this firing in three >> places. >> >> One of them is in the libc tests for memset and bzero, where the 0 >> argument is intentional. >> One of them is here: >> https://github.com/apache/xerces-c/blob/trunk/src/xercesc/util/XMLUTF16Transcoder.cpp#L114 >> (note that this code is correct). >> And one of them was a real bug where the second and third arguments of >> memset were transposed. >> >> That's an extremely low false positive rate, much lower than what we'd >> expect for an enabled-by-default warning. I find this extremely surprising; >> I would have expected the ratio of true-- to false-positives to be much >> much higher. But ultimately data wins out over expectations. >> >> How does our experience compare to other people's experiences? Are our >> findings abnormal? (They may well be; our corpus is very C++-heavy.) If >> this is indeed typical, we should reconsider whether to have these warnings >> enabled by default, as they do not meet our bar for false positives. >> > > I am confused by the "low/high" and "meet/do not meet" in this comment. > > Is this warning currently enabled by default? > It's currently enabled by default. Is it currently enabled by -Wall? > It's not controlled by -Wall / -Wno-all. > Are you saying that "2-in-3-firings false positive" is sufficiently high > that this warning should be disabled by default? > Yes, absolutely, that's *way* too high a false-positive rate for an enabled-by-default warning. > Are you saying that "2-in-a-gazillion-lines false positive" is > sufficiently low that this warning should be included in -Wall? Are you > saying something else? > How often the warning fires in total is not especially important. What matters is how much signal it provides that there is a problem in the code. > My experience at Green Hills many years ago was that this warning caught a > ton of real bugs; but I don't remember if it was "on by default" or in our > equivalent of "-Wall" or what. I think it should be enabled by -Wall (or > -Wmost), and have no opinion as to whether it should also be on-by-default. > As I said, my expectation is that this would be a good warning to have. But data wins, and the data I had did not back up this expectation. Erik's additional data helps a lot to justify this warning being enabled by default.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
