r337470 - [Sema] Add a new warning, -Wmemset-transposed-args

[Richard Smith via cfe-commits]

On Thu, 26 Jul 2018 at 13:22, Arthur O'Dwyer via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Thu, Jul 26, 2018 at 12:52 PM, Richard Smith <richard at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180726/33bf9a9d/attachment.html>