[StaticAnalyser][RFC] New checker, -Wsign-compare without the noise

Hello!

I want to implement a new -Wsign-compare checker in the static analyser that is not noisy.

I like the idea of Wsign-compare. However the checker generates lots of noise:

    int x = 10;
    unsigned y = x; // <- FP

The -Wsign-compare doesn't care that the code is 100% safe and warns anyway.

I only want to see a warning when there can be a real loss of precision or a real loss of sign (when the value can be negative).

I don't plan to remove the -Wsign-compare from Clang. People will have to use -Wno-sign-compare to get rid of the noise.

I attach a simple proof of concept checker. It will just warn if there is an assignment and RHS is a known negative value. Do you have opinions about the design? Should some alternative approach be used?

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62
E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

150901-sign.patch (3.89 KB)

I attach a simple proof of concept checker. It will just warn if there is
an assignment and RHS is a known negative value. Do you have opinions about
the design? Should some alternative approach be used?

You seem to have substituted a lot of false positives for a lot of false
negatives.

Why not look to see if the value cannot be provably non-negative? That
would resolve the false positive you showed above, but still capture cases
where the case only *might* be negative (rather than being guaranteed to be
negative). In fact, I think the most interesting case is those where an
off-by-one error or some other hidden defect has caused a scenario where
almost all cases are positive, but left a couple cases open that the
developer did not realize could be negative. Warning in these scenarios
that the conversion could be unsafe would be very powerful, I think.

- Matthew P. Del Buono

I attach a simple proof of concept checker. It will just warn if there is an assignment and RHS is a known negative value. Do you have opinions about the design? Should some alternative approach be used?

You seem to have substituted a lot of false positives for a lot of false negatives.

Thanks for looking!

It is just a proof of concept patch. I wanted it to be accurate. The finished patch will have fewer false negatives for sure.

My primary goal is to avoid obvious FPs. When it is obvious that the signed value can’t be negative there should be no warning about loss of sign.

If that means the amount of FPs will be acceptable on real code, I am fine with that.

I have been told that we shouldn’t even warn for:

void foo(int x) {
unsigned y;
y = x;
}

… as developers often know that some function parameters are always positive. if we want to properly warn about that we should lookup all “foo” function calls and see if the parameter is negative sometimes.

Best regards,
Daniel Marjamäki

Daniel Marjamäki Senior Engineer

Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62

E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

Hi!

Having a check with less false positives is certainly useful. What is the state of this? Did you try it on some projects? Did you get promising results?

Regards,

Gábor

It was committed: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

I would say it’s not finished.

I saw true positives with this checker in some open source projects. I assume the Wconversion had been deactivated because it’s too noisy.

In a comparison I made last year between PC-Lint and Clang, PC-Lint wrote 100’s of conversion warnings and Clang didn’t write any. I did not see a single “real” issue where the code was actually dangerous so I classified all those warnings as false positives.

Daniel Marjamäki Senior Engineer

Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62

E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se