<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, 26 Jul 2018 at 13:22, Arthur O'Dwyer via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">On Thu, Jul 26, 2018 at 12:52 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">Other than the (fixed) ffmpeg false positive, I see this firing in three places.<div><br></div><div>One of them is in the libc tests for memset and bzero, where the 0 argument is intentional.</div><div>One of them is here: <a href="https://github.com/apache/xerces-c/blob/trunk/src/xercesc/util/XMLUTF16Transcoder.cpp#L114" target="_blank">https://github.com/apache/xerces-c/blob/trunk/src/xercesc/util/XMLUTF16Transcoder.cpp#L114</a> (note that this code is correct).</div><div>And one of them was a real bug where the second and third arguments of memset were transposed.</div><div><br></div><div>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.</div><div><br></div><div>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.</div></div></blockquote><div><br></div><div>I am confused by the "low/high" and "meet/do not meet" in this comment.</div><div><br></div><div><div>Is this warning currently enabled by default?</div></div></div></div></div></blockquote><div><br></div><div>It's currently enabled by default.</div><div><br></div><div><blockquote class="gmail_quote" style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>  Is it currently enabled by -Wall?</div></div></div></div></div></blockquote><br class="gmail-Apple-interchange-newline">It's not controlled by -Wall / -Wno-all.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Are you saying that "2-in-3-firings false positive" is sufficiently high that this warning should be disabled by default?</div></div></div></div></blockquote><div><br></div><div>Yes, absolutely, that's *way* too high a false-positive rate for an enabled-by-default warning.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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?</div></div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div></div></div></div></blockquote><div><br></div><div>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.</div></div></div>